Add "Save as..." feature for dictionary conversion

[user202729]

Summary of changes

Right-click at a dictionary and choose "Save as..." can make a copy of the dictionary.

As far as I know, there's no existing easy way to do it natively with Plover. (#589, https://groups.google.com/forum/#!topic/ploversteno/PRapWs8I2-o, https://github.com/balshetzer/plover_dict_converter, http://plover.stenoknight.com/2012/12/dictionary-converter.html -- although all of those are at least 4 years old)

Issues:

• Single quote? (it's just copied from the function above)
• Should the feature be disabled if there are more than one dictionary selected? Currently it will just save whichever dictionary being clicked on.
• Saving a Python dictionary will just produce an empty dictionary. (however it's not just an issue with this feature, editing a Python dictionary (by double-clicking at it) brings up the dictionary editor with no entry too)
• Is the wording misleading? The new dictionary is not used in place of the old one. Perhaps "Save a copy as..." instead?
• Perhaps a button on the toolbar below too?
• The except: is also copied from the function below. I'm not sure which function can raise the exception.
• Are the type hints okay? Which Python version must be supported?
• By the way, the types of

        self._config_dictionaries = {}
        self._loaded_dictionaries = {}

are really inconsistent. At least use None.

Pull Request Checklist

• Changes have tests
• News fragment added in news.d. See documentation for details

[morinted]

Thanks for this feature, I've been thinking about it for a long time!

• I personally like type hints, but a while back we made the decision to not include them in Plover. Until that changes at a later date, let's just remove them from this PR
• It would be nice to disable the feature when you've got multiple dictionaries selected
• I do prefer the wording "Save a Copy" or "Export a Copy" because it's possible to convert the dictionary at this point by selecting a different format and it won't be added to the list of active dictionaries
• Since we're defining a context menu, it would be nice to also include the existing actions → Edit, Remove, Disable/Enable for consistency's sake. We could move this into a new issue, though, rather than scope creeping this PR.
  • Speaking of scope creep, I really want an "open containing folder" in the context menu of dictionaries.
• I think the default name in the file dialog should be populated—maybe just to the existing name and extension.

[user202729]

A weird corner case is that the user saves a copy to overwrite some other dictionary in the list.
In that case should Plover reload that dictionary, or just disallow the user from doing that?
(currently it doesn't reload.)

[morinted]

It would be good to do a quick config refresh which should trigger any dictionary changes from the file system. I don't think stopping someone from exporting onto a dictionary that they have open makes sense (even if that use case is weird as heck.)

[user202729]

Another weird edge case is that between the right-click and the save, some plugin might change the dictionary list so the dictionary at that row may point to a different one or missing (index out of bound). What should be done in that case? (report an error? Save the old one?) Similarly, what if some plugin adds a dictionary entry into that dictionary after the right click but before the save?

By the way, the internal functions are really poorly-documented. What is the loaded_dictionaries parameter of _update_dictionaries used for, and how is that related to reloading the dictionaries (as used in _create_new_dictionary)?

[morinted]

Maybe we should grab reference to the dictionary on context menu selection rather than just using the index. Wholeheartedly agree that the code documentation needs love and care. It's very unfriendly to new (and old) devs.

…

On Fri., Oct. 16, 2020, 11:35 p.m. user202729, ***@***.***> wrote: Another weird edge case is that between the right-click and the save, some plugin might change the dictionary list so the dictionary at that row may point to a different one or missing (index out of bound). What should be done in that case? (report an error? Save the old one?) Similarly, what if some plugin adds a dictionary entry into that dictionary after the right click but before the save? By the way, the internal functions are really poorly-documented. What is the loaded_dictionaries parameter of _update_dictionaries used for, and how is that related to reloading the dictionaries (as used in _create_new_dictionary)? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1149 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMSASWZ23O2RHNFGRS36E3SLEGJFANCNFSM4STN4GKQ> .

[morinted]

Can you add a news fragment please?

[user202729]

• In retrospect, creating and deleting a QMenu and QAction every time a right-click is performed is not a good idea.
• Perhaps "Save As..." is more common, where the new dictionary replaces the old one in the dictionary stack.

[user202729]

Given #922, perhaps it actually makes sense to allow export multiple dictionaries, with higher priority dictionaries preferred.

[user202729]

There's that new thing which is almost the same thing as the one in this pull request; however you can roll it back if you don't want it.

Remark: currently "save a copy as" creates a new undo event; even though there's no dictionary change.

Should test with bottom-up dictionary order too (however currently there's no automated GUI test)

• Apart from "Save as...", what about "Convert to format..."? Or "Rename..." (remove the old dictionary, must have the same format)?

[benoit-pierre]

OK, so I took a look at this, and immediately there are some warning bells, with the way some Qt objects are dynamically created, combined with popup.popup vs popup.exec_ (asynchronous vs synchronous): if a reference to a Qt object is not kept and garbage collected by Python, or if deletions are not done in a certain order, you can get crashes on the Qt side.

First thing I did when testing, is I tried to save "main.json", on "main.json" (yes, vicious), and Plover crashed, not with a traceback, but flat out segfaulted.

But my biggest issue with this PR is the interface: it's inconsistent. Suddenly, we have this context popup, with only one entry, and it's not even for the most common operation. Add to that the dynamically changing text (save vs merge), which IMHO is bad for discoverability.

Some of the other issues are easy to correct:

Remark: currently "save a copy as" creates a new undo event; even though there's no dictionary change.

If you use engine.config = {}, the engine will automatically reload externally modified dictionaries, and you wont have this bogus undo event.

Should test with bottom-up dictionary order too (however currently there's no automated GUI test)

That part is taken care by _get_selection, which always return dictionaries in the search order. And yes, a way to test the GUI would be great.

See #1244 for an alternative.