Improved UI for Search/Replace

[idea00]

Hi,

I'm working on an improved UI for Search/Replace that adds the following capabilities:

- Unlimited search/replace definitions instead of just the fixed three.
- Ability to save/load the definitions for subsequent use.

The code can be found at:
lp:~idea00/calibre/search_replace

but is not yet ready for merge (have a few questions below.)

A screen shot of the UI implementation can be found at:

http://img37.imageshack.us/img37/9321/searchreplace.png


Rationale
The reason I made this change is that I have several badly converted epubs with leftover PML markup (\aXXX characters the full list is in PML Characters) and was converting them manually (unzip, search/replace with a text editor, zip.)

So I thought:
"This is stupid, I should just write a script to do this"
"Wait, this is a problem others may have, maybe I should add this functionality to Calibre"
"How? Where? Lets look at the develop docs."
"Ahh... a plugin!"
"Wait... Wait... Wait... before I write a new plugin lets make sure this has not already been implemented."
"Ok.. Calibre conversion settings... look and feel, heuristic processing... search and replace,..."
"What was that SEARCH AND REPLACE? Calibre has search and replace?!"

Long story short I need more than three search and replace definitions and I'd like to be able to save them so I don't need to input them every time I need to do this conversion (or any other common one.)
Now for the questions:

Backwards Compatibility
I changed the options 'sr1_search', 'sr1_replace', 'sr2_search', 'sr2_replace', 'sr3_search', 'sr3_replace' with a single 'search_replace' option (plumber.py and cli.py). This means any plugins that modifies the old search/replace options will not work, but I found no evidence of a plugin changing the user preferences so I believe this is okay. I think I can make my change backward compatible by introducing srX_search/srX_replace options dynamically based on the number of actual search/replace definitions, but I'd probably need to make substantial (ugly) changes.

Internationalization
I'm guessing the underscore function '_()' used around strings is the hash look-up for the language specific string, but I haven't deciphered this fully so pointers would be welcome. Specifically how are strings in the *.ui files handled and what else needs to be done to add new strings.
I used the help string for "SEARCH AND REPLACE" in 'cli.py' as the help string in the OptionRecomendation for the new search_replace option so that should be okay, but now the old help strings for srX_search/srX_replace are just laying around unused so are there any tools for cleaning up unused strings or should this be done manually (and how?)

User Experience
The new UI works by adding the search/replace definitions to the table widget. This means a user may enter (and test) a search/replace definition in the RegexEdit/Textbox widgets and forget to press Add.
There are two ways I can think to solve this:

1. Leave it as it is, the user will learn.
2. Pop a dialog asking the user whether to add the definition in the RegexEdit/Textbox widgets if it not already there when the leaving the Search/Replace window.

Anything Else
Anything else I need to do to make this change merge worthy?

[kovidgoyal]

Backwards compatibility:

You cannot remove the sr?_replace options. calibre saves previously used conversion settings. Your change would render the saved settings useless. Instead leave them be and add another option. Have the GUI use the new option for new conversions, pre-populating it with the values from the sr? options, if they exist. In the case where values exist for both, ignore the old ones.

Internationalization

You only need to mark strings with _() in .py files. .ui files are handled automatically. That's all you need to do.

UI

Why not use both the explicitly added expressions and the values in the textboxes, if they exist.

CLI

How are you serializing multiple expressions on a single command line option?

[idea00]

Originally Posted by kovidgoyal

Backwards compatibility:

You cannot remove the sr?_replace options. calibre saves previously used conversion settings. Your change would render the saved settings useless. Instead leave them be and add another option. Have the GUI use the new option for new conversions, pre-populating it with the values from the sr? options, if they exist. In the case where values exist for both, ignore the old ones.

I need a bit of help with this as I do not entirely understand how options are handled.
As far as I've gathered the options in the UI are somehow connected to the OptionRecomendations in plumber.py using the functions get_value and set_value in the class Widget (gui2\convert\__init__.py)
I added support for handling QTableWidget widgets to this class and named my table opt_search_replace so it matches the option name (search_replace)

In order to support sr?_search/sr?_replace options I'd either need dummy widgets or would have to use a different mechanism than the one used in the original search_and_replace.py. Can you point me in the right direction for this?

Originally Posted by kovidgoyal

UI

Why not use both the explicitly added expressions and the values in the textboxes, if they exist.

That's the less intuitive option, it means if a user just messes around with search/replace and decides not to use it, he now needs to delete the regex from the RegexEdit widget not just remove everything from the table.

Originally Posted by kovidgoyal

CLI

How are you serializing multiple expressions on a single command line option?

Forgot to ask about this.
I use json.
Widget.get_value gets the text values of the table into a list (of lists) and json encodes it.Widget.set_value reverses the process.
The save and load load functions use the same functions to serialize/deserialize the search/replace definitions into a file.
A bit ugly but less computationally intensive than dynamically generating search and replace options. The original approach of using two options for what is in essence a single function was uglier still as one could potentially disable just a replace option without disabling the matching search option.
The best approach would be for options to support lists natively not just numbers and strings, but I have not yet deciphered how to do it or know the implications of such a change.

[kovidgoyal]

Don't change the Widget class. Instead implemnt get_value_handler and set_value_handler in the S&R class to special case the handling of this option. You can be as flexible as you want in those methods. A grep in that directory will show you examples of using those methods.

On the GUI:

There are two less than optimal scenarios:

1) The user enters something but does not click add. I which case his expression is ignored
2) The user enters something but does not click clear, in which case the expression is applied even though he did not intend it to be applied.

To me, scenario (1) is preferable, as it makes the feature easier to use for newbies. You can always add a popup that pops up only if there is text in those boxes and asks the user if he wants to apply that text or not. Do these by subclassing commit() in the S&R widget.

On the CLI:

You wont be able to find a serialization scheme that is both robust and easy to enter given the quoting restrictions of most shells. Instead, I suggest using the path to a file for the CLI.If specified, load the expressions from that file. To make it human usable have the file format be something simple like

single line regex1
replacement expression1

single line regex2
replacement expression2

This also allows for an easy analogue of the save load functionality from the CLI. You will also be able to use this option in the GUI, by writing your table into a temp file and passing it using this option.

[idea00]

Originally Posted by kovidgoyal

Don't change the Widget class. Instead implemnt get_value_handler and set_value_handler in the S&R class to special case the handling of this option. You can be as flexible as you want in those methods. A grep in that directory will show you examples of using those methods.

The methods are barely used by the classes in that directory, but looking at Widget again I see how they work. The methods still get passed a property of the main class (the g parameter) not the name of the desired option.
I'll still need the opt_sr* properties but I can get it to work by saving the name of the option in the property.

I'll revert the changes to Widget and implement: get_value_handler, set_value_handler and connect_gui_obj_handler.

Originally Posted by kovidgoyal

On the GUI:

There are two less than optimal scenarios:

1) The user enters something but does not click add. I which case his expression is ignored
2) The user enters something but does not click clear, in which case the expression is applied even though he did not intend it to be applied.

To me, scenario (1) is preferable, as it makes the feature easier to use for newbies. You can always add a popup that pops up only if there is text in those boxes and asks the user if he wants to apply that text or not. Do these by subclassing commit() in the S&R widget.

I was thinking on the lines of less damage. Scenario 1 may cause the user to loose text from books should the original be deleted after the (erroneous) conversion, scenario 2 will merely not do the job but no data will be lost.
In any case I'll go with the popup option which is the best and also clear the fields for search/replace in the RegexEdit and Textbox when the user adds or changes a S/R definition in the table.

Originally Posted by kovidgoyal

On the CLI:

...

Sorry just now the acronym CLI (Command Line Interface) clicked. I thought you where referring to something else in your previous post and hence the answer.
I'm going to restore backward compatibility so the CLI for all the original sr* options should be restored and use a single human parsable serialization scheme for both the UI saved files and the CLI.

[idea00]

Ok I finished with all the open issues and sent you a merge proposal on lp.

Backward Compatibility
Fully backwards compatible any sr?_search / sr?_replace option saved in the conversion definitions are automatically converted to entries in the search_replace option and they can be used in ebook-convert as usual.


User Interface
Reverted Widget and implemented: get_value_handler, set_value_handler and connect_gui_obj_handler.
I still needed to change Widget to include setup_help_handler (as a counterpart to setup_help) in order not to have to use dummy widget for the original sr* options.

CLI

Originally Posted by kovidgoyal

You wont be able to find a serialization scheme that is both robust and easy to enter given the quoting restrictions of most shells. Instead, I suggest using the path to a file for the CLI.If specified, load the expressions from that file. To make it human usable have the file format be something simple like

single line regex1
replacement expression1

single line regex2
replacement expression2

This also allows for an easy analogue of the save load functionality from the CLI. You will also be able to use this option in the GUI, by writing your table into a temp file and passing it using this option.

I can't just use a temporary file for the option as the options are saved for each ebook and for bulk conversion when using the GUI so I'm using a dual approach: json or a file can be accepted.
The GUI will always return json so it can be saved along with the other options and used during conversion without requiring an external file.

I hope you have time soon to review my proposal and that it can make it to the trunk.

[kovidgoyal]

Merged, changing to remove unneccessary file/json duality and also some miscellaneous cleanups/fixes. I still have to test backward compatibility, but otherwise it looks good.

[idea00]

Thanks.
I saw what you did, nice.
If there are any bugs I need to take care of email me.

[_jago]

I use the S&R funktion frequently and appreciate the new version very much. Thank you for that I love it.
But if I am not totally wrong, the execution order of the regex seems not predictable and that's bad for some special cases.
I would have expected that the execution is in order from 1 to N.
Can this be changed? And if so it would be nice to have the easy posibility to change the position of every regex in the stack (e.g. up/down arrows).

[kovidgoyal]

IIRC, the order is preserved. Try emailling idea00 to see if he is willing to implement the order movement buttons. You will find his email address in the calibre source code file gui2/convert/search_and_replace.py

[idea00]

Merge request sent.

I've added reorder arrows to the UI and fixed a few minor UI logic bugs.

The order in which the S/R expressions are applied in the current version is reverse (hence _jago stating the order does not seem predictable.)
The reason is the function "do_search_replace" in preprocess.py inserts s/r rule at the start of the rules list.
I assume there is a reason for this so I fixed this by using a revered() iterator, if not appending to the rules would also solve this issue.

[_jago]

Wow! Didn't expect such a fairly quick response to my suggestions. Thank you for your effort to make this nice funktion much more usefull for me (hopefully not only for me).

The order in which the S/R expressions are applied in the current version is reverse

Ok, that explains my confusion with the execution order.

But anyway as soon as possible I will test the new functionallity.