Conversation
|
This pull request introduces 1 alert and fixes 5 when merging 974edcb into d867fe0 - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request fixes 5 alerts when merging 7c11e1b into d867fe0 - view on LGTM.com fixed alerts:
|
| #ifndef _NO_PYTHON | ||
| GMenuItem2 *t = FVGetToolsSubmenu(); | ||
| FVSetToolsSubmenu(NULL); | ||
| #endif |
There was a problem hiding this comment.
What side effect are we trying to bypass here?
There was a problem hiding this comment.
Ah -- The localization of the python sub-menu is handled differently and we don't want mb2DoGetText() to write over it for us.
I wanted to handle this by pre-setting .ti.text_untranslated to non-NULL and modifying mb2DoGetText() to only set the fields if text_untranslated is NULL. I believe the reason I couldn't do that is because all of the static gtextinfo initializers, including those in all the menu entries, poop out before the text_untranslated field (which was added later). Therefore there's often garbage in the field and you can't use its initial value for anything. *sigh*
There was a problem hiding this comment.
(maybe the menus aren't a problem because they're static, but I know I ran into it in some cases.)
| static char *SetMnemonicSuffix(const char*menu_string, unichar_t c) { | ||
| char *r = mncopy(menu_string, NULL, 4); | ||
| int l = strlen(r), i=l-1; | ||
|
|
There was a problem hiding this comment.
Are you sure about the bounds-checking here? We can't count on a null terminator going backwards. If r is "", it seems like we might get a segmentation fault checking *(r+i) with i = -1. r = " " and other edge cases also seem likely to cause problems.
There was a problem hiding this comment.
I rewrote this a few times and am trying to remember why it wound up this way.
Clearly if menu_string is NULL you're already in trouble, so I guess I should return an extra-length zeroed out string in that case. That would also make strlen() happy.
For the rest of it could I just add i>=0 && to all the guards before the last one?
There was a problem hiding this comment.
I improved this some. registerMenuItem() rejects null name strings but I confirmed this works with empty strings and does something appropriate with all-space strings and trailing strings. It also passes my old test sample.
|
This pull request fixes 5 alerts when merging c80a572 into a5dedb4 - view on LGTM.com fixed alerts:
|
jtanx
left a comment
There was a problem hiding this comment.
So I did stop reviewing once I hit mncopy, but just posing a design question overall:
To confirm: on e.g. "Annuler (4)"; - is that assigning it the mnemonic of 4 based on its position in the menu?
imo in general this feels like an over-complication of matters, in that I don't understand what's so bad about just doing a best effort of assignment of mnemonics (i.e. how much benefit there is over #4498 in terms of mnemonic selection) - and in particular, why this (python scripting menu items vs in-built menu items) needs to be treated so specially to everything else.
Also the ascii restriction feels like an over-restriction/simpification too. u2utf8_mncopy deals with extracting unicode mnemonics just fine, and gdraw not activating on such entries is possibly more of a gdk regression, or at least a result of comparing against keysyms instead of possible unicode values (I've just been delving into this, relates to #4712).
| free(fn); | ||
| } | ||
|
|
||
| static void hotkeysSaveCallback(Hotkey* hk,FILE* f) { |
There was a problem hiding this comment.
Just curious (I know, not directly relevant to this change), did this use to do something more in the past, or is this solely to just print the values to the console?
There was a problem hiding this comment.
I believe that hotkeysSaveCallback() was speculative generality anticipating a UI for changing hotkeys. It writes out the entries back to the user's hotkey file. I disabled it in #3606 because automatically writing over a file meant to be hand-edited is annoying.
| } | ||
|
|
||
| void HotkeyParse( Hotkey* hk, const char *shortcut ) { | ||
| int HotkeyParse( Hotkey* hk, const char *shortcut ) { |
There was a problem hiding this comment.
So this returns true if either no shortcut (was intentionally) set and/or was successfully set, and false on failure?
There was a problem hiding this comment.
Yeah -- false means (roughly) "couldn't parse".
| ret = dst = malloc(l+1+extra); | ||
| for( ; *src; src++ ) { | ||
| if( *src == '_' ) { | ||
| t = *(src+1); |
There was a problem hiding this comment.
This is going to break if the following character is not ascii. Also noting the lack of the use of utf82u_mncopy
No -- When the requested mnemonic is taken or there is no requested mnemonic the code just works through a hard-coded list of potential options, ordered roughly from least likely to most likely (to increase the chance that later entries get their requested mnemonic). It assigns the first one it finds free.
The answer to this last question is straightforward: All other menus in the project have, or are likely to have, static entries relative to the localized language (which can be adjusted as needed). This (likely) includes Tools submenus. That means either we or the submenu designer can lay out the mnemonics in advance in a way that makes sense. The Tools menu, in contrast, is composed of whatever the plugins and user have put in there, in whatever order they were added or the user picked. The developer of plugin Foo can't count on their requested mnemonic being available because they can't know what else has been added to the menu before their entry. Some users prefer mnemonics, and this was the way that I came up with that was "stable" (relative to the entries from the start, anyway) and that would assign a mnemonic to every entry.
I don't think non-ascii options work as accelerators, or at least when @frank-trampe, @ctrlcctrlv and I discussed this on the phone it didn't seem like an option. These are typically activated by Alt and accents and such are also often entered that way. (Or, however they're entered with the keyboard is likely to conflict.) The usual convention seems to be picking a salient letter in the case of other Latin-encoded languages and adding the English mnemonic in parentheses in non-Latin-encoded languages. If you start up FontForge in Korean ( |
Yep, I'm aware of that convention. Having said that, reading the gdraw code I do believe it was done with the intention of supporting more than ascii, as per how I was just playing around with it, and it can work: https://github.com/fontforge/fontforge/compare/master...jtanx:gms?expand=1 That also fixes #4712 (at least on KDE+Windows, Mac seems like something else is needed, not sure if gdk supports it). It comes down to the mnemonic checks being done against the |
|
I think that URL is screwed up. |
Basically this a1e4732
Yeah, I guess that's where I'd personally find this use questionable/overkill if the mnemonic assigned can vary depending on what's already present |
|
If it's questionable/overkill what is the alternative? Is it preferable to have a mnemonic or not have one depending on what's already present? |
|
Basically just try to set it and give up if it can't be set, same as what happens if you assign more than hard-coded mnemonic. That is as far as I'm aware, consistent with how Windows deals with duplicate/clashing mnemonics (and I assume a similar story for other toolkits). |
Do other programs do this? Do users of non-Latin scripts expect or want non-Latin accelerators? I was under the impression that the current FontForge convention follows normal practice. |
|
I don't see why not, it is entirely possible to not have a Latin keyboard at all. Just tested on Windows and it works fine using a Czech keyboard layout: |
In most cases a conflicting mnemonic is a configuration error that would be fixed by changing the sequence. @frank-trampe, @ctrlcctrlv and I had an audio conference before I started on this work and we settled on this approach. The code may be more complicated but it's implemented. The user can still override the mnemonics if they want to. I don't see how its a sin to put one there otherwise. |
So then it should be fairly easy to turn up a few examples, thus actually answering the question. |
I guess my thinking is, apart from what I've already stated, this is now 100% FF specific behaviour that can't be found in any other toolkit, and also doesn't really follow the principle of least surprise (instead of assigning/trying to assign the given mnemonic, it chooses some other) |
I just did a search on https://github.com/microsoft/vscode-loc with And yes there definitely are. e.g. https://github.com/microsoft/vscode-loc/blob/889ea08276f2afc0dca06b5e6d9563faad4eb288/i18n/vscode-language-pack-tr/translations/main.i18n.json#L1407 The Russian one is probably a better example, but it's too large to render in github. But e.g. |
|
|
Wait, why are we using Czech as an example of non-Latin? OK: It's possible to use accents. That's not a central concern relative to our discussion. The question is whether there are Greek, Cyrillic, etc accelerators. |
|
OK, the Russian example is better. |
Sure, but mnemonic functionality is not specific to FontForge, and as far as I know, nothing else will pick a new mnemonic for you if you have a clash. Having a clashing mnemonic should be fairly easy to see and reason about. If we're saying that such scripts are already quite rare, then I find it even more unlikely that there would be a clash in the first place. |
|
I'm not saying that scripts are going to be rare -- the whole point of this plugin push is to increase the number of scripts available. I'm saying that programs with plugin interfaces that add menu items are rare. If you want to argue about these conventions then that is the proper comparison class. |
|
I'm not going to have time to overhaul this PR in the short term. I'm not giving up on the design just because you see it as overwrought. It shouldn't be changed without reinvolving the interested parties (@ctrlcctrlv may not care anymore). So I guess you can feel free to stop your review. |
For who to reason about? Not all users are the sort to read the docs and break out their ASCII editors to configure their own shortcuts and mnemonics. Not all users even know how to use an flat file ASCII editor, even those who prefer mnemonics. Your answer just punts to the end user. |
I mean, even by this logic, there shouldn't be all that many entries in the menu and so the chance of a collision is relatively rare.
Maybe I'm missing something entirely, but is the script author not already choosing the mnemonic via the leading underscore? Yes, it is a punt, but no more so than what all other toolkits would do. In terms of reasoning, like it's pretty clear to see that say this happened: You can see that both have the same mnemonic (o), and if you do activate the mnemonic, quite predictably, it chooses the first that appears (Reencode). That to me is easy to reason about.
Yep, I'm happy to leave this here for now. |
I don't see how this follows, unless you're assuming global knowledge on the part of plugin authors.
You're just continuing to ignore how this case is different from other cases. |
Is this wrt. not having a built-in default/not knowing in advance what mnemonics would be used? OK, even assuming that there will be lots of menu entries, and potentially lots of clashes, I'd still say that assigning a random (to the user at least, no one is going to look into what algorithm it uses) mnemonic is going to cause more trouble than it's worth. - given it has the potential to vary between users and even depending on what plugins are used/enabled at the time. At best, it is maybe usable supposing that key is available on the user's keyboard and supposing they keep exactly the same set of plugins/calls, and at worst, confusing if it differs between people(+enabled plugins), to unusable if the key doesn't even exist. Outside of a complete redesign of everything, I still think a best-effort assignment remains the most practical. For comparison (I think I saw something similar), with vscode extensions (my memory is a little rusty), you assign a name to an action (like |
|
The mnemonic improvisation logic (almost completely contained in |
|
The L-1011 shipped with exterior doors that lifted up and out of the way (balanced by springs). The feature was unique at the time and never widely adopted, but it was handy and stylish. If we say that this feature is unique, I see it in the same light as the L-1011 doors. The few people who notice it are going to be mildly impressed rather than befuddled. |
|
I've been thinking about this PR again and how to finish it. I'm going to assume that the general Tools menu alt-key assignment convention can be accepted. In that case it seems that the main problem with the PR as it stands is that it's too latin-centric. When I wrote this I had thought that Latin characters were the general solution but it appears that some languages, particularly Russian, use other characters. This raises a number of problems. I'm currently thinking of a solution something like this:
Most of this processing seems a whole lot easier to manage with unichar_t strings than utf8 strings so that's probably the direction I'll head down. |
|
That should probably be "ToolsMnemonicOrder". |
|
I pushed my proposed changes minus the preferences support: You should now be able to specify any sequence of Unicode characters as Tools menu mnemonics by internationalizing the default string. I would have liked to add this as a preference but the semantics of that system make it difficult. Ideally opening the window would display the default string for the current initialization but nothing would be saved unless the user changes its value. I can't figure out how to implement that in the current system and I'm not inclined to do yet more special-casing (e.g. with BDFFoundry now). |
|
This pull request fixes 5 alerts when merging 77ab09f into 6482224 - view on LGTM.com fixed alerts:
|
|
I remain in favor of merging this and believe that the increase in utility overwhelmingly offsets the increase in complexity. @jtanx? |
|
I'll take a look at this again in the next couple of days |
|
I've updated this with @jtanx's suggestions. |
jtanx
left a comment
There was a problem hiding this comment.
Been a while since I last looked at this, but I still feel like the mnemonic stuff in particular is overcomplicating matters when #4498 (comment) would get you 95% of the way there, excluding the auto-substitution of a free mnemonic.
I can get behind the rest, particularly the hotkey support, but just some questions on the need to provide both localised and non-localised strings
| windows created after this command is executed. Normally the command will | ||
| be executed at startup and so it will affect all windows. | ||
| ``name`` can be a string but ideally it is a tuple of three strings | ||
| ``(localized_name, english_name, identifier_string)`` or of two strings |
There was a problem hiding this comment.
Why is it necessary to specify the English name? The hotkey system (questionably) looks for shortcuts based on the untranslated text from the hotkeys file, so I can somewhat understand that, but all of these hotkeys are 'volatile' in that they are registered every time this function is called. Perhaps I'm missing something?
There was a problem hiding this comment.
Alternatively if it is somehow a requirement for hotkeys, why not just use the identifier (which imo, is what should have been done for all hotkeys; assigning hotkey identifiers instead of relying on untranslated strings)
There was a problem hiding this comment.
The hotkey specified at registration time is just a suggestion -- it has lowest priority compared with FontForge code hotkeys (which may be added to over time) and user-defined hotkeys.
The English string is for consistency in the user overriding (or just choosing -- I expect most registered menu items will skip the hotkey suggestion) the choice using their hotkeys file, which (for reasons that can be questioned but probably aren't changing anytime soon) are always overridden with the English/code-native strings.
There was a problem hiding this comment.
The reason not to use the identifier is consistency with the existing infrastructure, which doesn't use identifiers anywhere else.
There was a problem hiding this comment.
Given that the identifier is required to be provided, can that not be used instead?
There was a problem hiding this comment.
(Apparently longstanding): 'Font' context menu items are not added/enabled until a new fontview is created (either by opening a new font or creating a new one)
The font menu context problem mirrors most of the rest of the UI: If you change things you typically don't see the result until the next time you open a window.
The 'Tools' menu is permanently in the 'Enabled' state, even when it has no items
I noticed the tools menu thing. I'll try to fix that.
Auto-assignment of mnemonics only happens at the first level, not the submenu
The auto-assignment of mnemonics is intentionally limited to the first level, as noted in the earlier discussions. A sub-menu "collision" is of course possible but for the most part plugin developers will add their own independent submenus (and are encouraged in the plugin documentation to put additions in a submenu). When you know the contents of a submenu there's no possibility of collisions and therefore no need for auto-assignment.
If I rerun the same registration code, I end up with two distinct entries instead of overwriting the first. Sample
Having unregister/reregister work better would be nice but seems unnecessary for this PR given that there's no change.. But I'll look at it.
Assigning a hotkey only works if I omit mnemonic underscores, otherwise no such hotkey is assigned:
I think is how hotkey assignment works in general. I suppose we could remove underscores in the passed string to improve things, as long as we were confident they aren't being removed in names. (Or I guess we could pass the passed string through one of the mncopys?)
Non-mnemonic parenthesised character is stripped:
I can try to fix this.
I'm a bit busy before Wednesday and maybe next weekend so I may need until then to look at what I've said I will.
There was a problem hiding this comment.
The font menu context problem mirrors most of the rest of the UI: If you change things you typically don't see the result until the next time you open a window.
Sure, I'm just saying that it's not intuitive or immediately apparent. But given it's longstanding it's fine to leave.
The auto-assignment of mnemonics is intentionally limited to the first level, as noted in the earlier discussions.
My points from my prior comment remain the same
Having unregister/reregister work better would be nice but seems unnecessary for this PR given that there's no change.. But I'll look at it
I'm not quite sure I understand this, but as is, there is no way to add more than one entry to a submenu, which is broken and needs fixing before this can be merged. It's at least a definite regression from behaviour prior to this PR
I think is how hotkey assignment works in general. I suppose we could remove underscores in the passed string to improve things, as long as we were confident they aren't being removed in names. (Or I guess we could pass the passed string through one of the mncopys?)
I thought the mnemonic-stripped string was already being passed to the hotkey system, although admittedly I didn't look too deeply into the exact cause
There was a problem hiding this comment.
I thought the mnemonic-stripped string was already being passed to the hotkey system, although admittedly I didn't look too deeply into the exact cause
loadHotkeysFromFile() leaves action as-is and so does hotkeySetFull(), as far as I can tell. I think to set hotkeys at present you need to strip the underscores yourself. (I suppose we could also do something more clever than strcmp() in HKActionMatchesFirstPartOf().)
There was a problem hiding this comment.
The 'Tools' menu is permanently in the 'Enabled' state, even when it has no items
If I rerun the same registration code, I end up with two distinct entries instead of overwriting the first.
Non-mnemonic parenthesised character is stripped:
I think I've fixed the first two and greatly (greatly) reduced the chance of the third.
Take a look and also let me know where we are with this after these changes. The window-not-updating and hotkey-file-with-underscores issues seem consistent between normal and python menus. I think I've dealt with what I have to but you might disagree.
As far at the identifiers-only route goes, I'm not against in principle but is the implementation clear? E.g. how will the menu item wind up with the hotkey printed if there's no association with the menu hierarchy?
I don't think it's too bad, set the identifier as the untranslated text for the menu item
Menus exist in a hierarchy and identifiers don't. (I haven't thought much about submenu-pointing-entries and identifiers. But they're normally not needed or used that way.)
There was a problem hiding this comment.
Menus exist in a hierarchy and identifiers don't. (I haven't thought much about submenu-pointing-entries and identifiers. But they're normally not needed or used that way.)
I'm not quite sure I get the implication, but ok.
|
This pull request fixes 5 alerts when merging 265ee8f into 6192309 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 5 alerts when merging c75a306 into 0bf72b2 - view on LGTM.com fixed alerts:
|
|
So I still find some of the changes made questionable, and it is more complex than it needs to be, but functionally it works, and I'm not going to block the release for this. Worst case we can always change the interface down the line. |
* Improved python menu support * Make LGTM happier * Improve safety and generality of SetMnemonicSuffix heuristics * Draft of switch to unichar_t alt processing * @jtanx suggestions * Revert to using utf82u_mncopy() * Code review fixes * Wall fix
This does more or less what @frank-trampe, @ctrlcctrlv, and I discussed at the meeting a while back:
registerMenuItem()and it will be added if not already used.registerMenuItem()now has both a positional API and a keyword API. The former is forwards-compatible with previous versions of FontForge. There is also a short keyword-based API for adding a menu separator.Adding hotkeys to Tools menu items continues to work, although now that names can be localized these hotkeys work a bit more like the others. If there is no localized/untranslated pair the string (minus mnemonic marks) is used in both contexts. I've verified that you can assign a hotkey to an item with a Japanese name.
As usual no Unit tests for UI stuff but I've done a fair amount of testing, including eliminating a number of problematic mnemonic key candidates.
It's a shame that you can't assign a hotkey with either English or a localized string but that would be tricky to fix because of mnemonics-related-issues.
Closes #4638
Closes #4148
Closes #4147 (in conjunction with the already-merged #4642)