Add menu shortcut texts for non-darwin OS #5248
Conversation
widget/menu_item.go
Outdated
| @@ -37,6 +41,25 @@ var keySymbols = map[fyne.KeyName]rune{ | |||
| fyne.KeyUp: '↑', | |||
| } | |||
|
|
|||
| // key default text | |||
There was a problem hiding this comment.
This comment does not add any value over the variable name. Either remove it or write something more helpful.
There was a problem hiding this comment.
Thank you for taking the time to review my PR.
I will rework the comments.
widget/menu_item.go
Outdated
| ) | ||
|
|
||
| var keySymbols = map[fyne.KeyName]rune{ | ||
| // key symbols for darwin OS |
widget/menu_item.go
Outdated
| @@ -13,13 +14,16 @@ import ( | |||
|
|
|||
| // TODO | |||
|
|
|||
| // modifier symbols for darwin OS | |||
widget/menu_item.go
Outdated
| @@ -13,13 +14,16 @@ import ( | |||
|
|
|||
| // TODO | |||
There was a problem hiding this comment.
Would you mind removing this while at it? :)
widget/menu_item.go
Outdated
|
|
||
| // textsForShortcut return texts for rendering a shortcut for a specific OS. | ||
| func textsForShortcutAndOS(s fyne.KeyboardShortcut, th fyne.Theme, os string) []*canvas.Text { | ||
| var b strings.Builder |
There was a problem hiding this comment.
I would rather keep the previous b := strings.Builder{} syntax if you don't mind. That's more consistent with other code in the repo as far as I know.
There was a problem hiding this comment.
I chose this form, because it is unnecessary to initialize a string builder and I think this form is more Go idiomatic (e.g. this is what is used in the official go documentation. And you actually find both variants in the repo (e.g. here.
So I would prefer to keep this change, would that be ok with you?
There was a problem hiding this comment.
There is no code difference between the two as all Go variables, unlike in C, are initialized no matter what so there is no performance win here. I much prefer using := where possible as there is less words to parse until I get to the type.
There was a problem hiding this comment.
I am aware that both variants are equivalent and I understand that you personally prefer the other form. My point is that using var form is more idiomatic Go code. e.g. the Go standard library uses the var form exclusively.
There was a problem hiding this comment.
I see your point and I appreciate that the compiler does something thing else but let's keep consistent with our own code base please. https://github.com/search?q=repo%3Afyne-io%2Ffyne%20strings.Builder&type=code
widget/menu_item.go
Outdated
| if mods&fyne.KeyModifierControl != 0 { | ||
| b.WriteRune(runeModifierControlDarwin) | ||
| } | ||
| if mods&fyne.KeyModifierAlt != 0 { | ||
| b.WriteRune(runeModifierAltDarwin) | ||
| } | ||
| if mods&fyne.KeyModifierShift != 0 { | ||
| b.WriteRune(runeModifierShiftDarwin) | ||
| } | ||
| if mods&fyne.KeyModifierSuper != 0 { | ||
| b.WriteRune(runeModifierSuperDarwin) | ||
| } | ||
| r, hasSymbol := keySymbolsDarwin[s.Key()] | ||
| if hasSymbol { | ||
| b.WriteRune(r) | ||
| } | ||
| t := canvas.NewText(b.String(), shortColor) | ||
| t.TextStyle.Symbol = true | ||
| texts = append(texts, t) | ||
| if !hasSymbol { | ||
| t := canvas.NewText(string(s.Key()), shortColor) | ||
| t.TextStyle.Monospace = true | ||
| texts = append(texts, t) | ||
| } | ||
| default: | ||
| if mods&fyne.KeyModifierControl != 0 { | ||
| b.WriteString("Ctrl+") | ||
| } | ||
| if mods&fyne.KeyModifierAlt != 0 { | ||
| b.WriteString("Alt+") | ||
| } | ||
| if mods&fyne.KeyModifierShift != 0 { | ||
| b.WriteString("Shift+") | ||
| } | ||
| if mods&fyne.KeyModifierSuper != 0 { | ||
| b.WriteString("Super+") | ||
| } |
There was a problem hiding this comment.
This seems like quite a bit of code duplication. Can you somehow move the darwin rune constants to a separate file and define the values specifically on darwin and have the other values in another file? This way we could have runeModifierControlDarwin return the darwin value on darwin and "Ctrl+" on other operating systems.
There was a problem hiding this comment.
If you look more closely you will see that the code for each case is actually quite different. The former adds rune, the later strings. The former can add two texts with different formatting, the later only one.
My intention here was to minimize changes to the existing code. I would advise against mapping the shortcut text via platform dependent constants though. That would make this function harder to test and understand. But I am happy to look into combining the two key maps into one, which should allow me to reduce the lines of code quite a bit.
There was a problem hiding this comment.
If you look more closely you will see that the code for each case is actually quite different. The former adds rune, the later strings. The former can add two texts with different formatting, the later only one.
I'm very well aware of the differences but there are still a lot of similarity here. The four if-statements are checking the exact same conditions and a rune is basically a string of length one so those could possibly be combined into one.
My intention here was to minimize changes to the existing code. I would advise against mapping the shortcut text via platform dependent constants though. That would make this function harder to test and understand.
Writing tests that only run on specific platforms is done in many parts of the code base already but if it makes more sense to keep them together the;let's go with that.
There was a problem hiding this comment.
I rewrote the whole thing incl. the existing code in order to reduce "code duplication" as requested.
There was a problem hiding this comment.
There appear to be some flaky tests and I do not have the rights to re-run tests in this repo, but they are now passing in my branch.
There was a problem hiding this comment.
I'm sorry but there seems like there have been some communication mishaps as some information got lost in translation.
You didn't have to rework the code that much, I find it a lot harder to read now, sorry. What I was trying to say was that the four if statements were duplicated. What I meant was this:
- Move darwin rune constants to a
_darwin.gofile so they are used only when building for that architecture. Use single length strings and remove darwin specific naming. - The other ctrl+ string etc. for other architectures can be put in another file that only is compiled in on non darwin desktop os. Constants have the same names as corresponding darwin ones.
- Move the four if statements out of the darwin/other-os checks so they happen before that and use the constants from above (that switch depending on the os in use). The previous code you had in this function for checking operating system can be kept and de-duplicating the if statements (the compiler will remove code for other operating systems automatically).
- One test runs on darwin the other for other architectures. There should already be existing code/files for this (as parts of the menus are darwin specific already), I think.
Again, I'm sorry if this was unclear but it was what I was trying to hunt at with my previous messages.
There was a problem hiding this comment.
There are 19 platform dependent strings, not 4. Are you suggesting to map all of those via target specific code or just the modifiers?
As I tried to explain before, it is not only the strings that are different, but there also is different code per platform. Do you want me to put that platform specific logic in target specific files too or keep the current approach via runtime constant?
Would it not make much more sense to use one approach to handle all platform specific variations, not mix approaches?
Again, would it not be better to avoid the additional complexity that target specific code adds and keep everything together in one file (i.e. use the runtime constant approach for everything)?
With all due respect, but it appears to me that you are asking changes without having a complete understanding of the code. So in order to avoid any more miscommunication, I would very much appreciate it if you could take the time to read through the complete code (and my above questions) and then revise your comments accordingly.
There was a problem hiding this comment.
With all due respect, but it appears to me that you are asking changes without having a complete understanding of the code.
In general this seems like an unhelpful thing to say - maintainers of this project generally have a good handle of the code. If in doubt try to assume that.
I do not understand where the miscommunication exists but this diff seems to be getting longer somehow.
Previously there were 3 consts that defined the runes used. And I understand that there are now 19 strings. But even so the code complexity should not have needed to increase.
We already had the OS specific (for darwin) code through build flags which I am sure @Jacalz was indicating could be kept.
Personally I would have split this into two commits - the first moving from the old rune system to new strings (where darwin are 1-length and others vary) which should have been an almost direct replacement. And then a second commit introducing the idea that the key symbol/name lookup could be set per-system as well.
It is possible that I have over simplified here, and if so please do share why that wouldn't be desirable - but it seems like it would be the minimal approach.
There was a problem hiding this comment.
In general this seems like an unhelpful thing to say - maintainers of this project generally have a good handle of the code. If in doubt try to assume that.
I did not say that. I was referring to the code of my PR, not the existing code of the Fyne project.
I am sorry, but it is still not clear to me what approach you want me to implement.
Please allow me summarize the issue as I see it:
The baseline is my initial commit, not the latest commit (That is what I understand @Jacalz is referring to).
There are 4 kinds of things that have to be defined for each platform:
- 4 modifiers (string mapping)
- 15 keys (string mapping)
- text format for modifiers (symbol for mac, default for others)
- text format for keys (symbol or monospace for mac, default for others)
There are 2 approaches to define something platform dependent:
- A: target specific code / build tags
- B: runtime OS condition
There are 2 solutions being discussed:
-
I would suggest to use approach B for everything, because it is simpler and allows you to unit test everything. While with approach A you can only test the whole thing in platform tests (e.g. on Github).
-
@Jacalz latest request would use approach A for the 4 modifiers and approach B for the rest. I find it pretty ugly to mix both approaches in the same solution.
I would appreciate if you could confirm that 2. is really the solution you want me to implement.
|
Yes a runtime check might seem simpler (not sure how using runtime checks is easier to unit test TBH). P.S. do check that the font must be different before coding that in - I think that if you choose symbol it would fall back to the normal font if the item is not in fact a symbol. |
|
I entirely agree that A matches what we already have the best. It is good to be consistent with that and also not compiling in unnecessary code can be a win (sometimes also for readability). To be fair, I did actually suggest B initially because that better described what I meant with "moving out the four if-statements" and because it was a smaller change from the existing code in the PR. However, I do agree that A is more consistent with what we already have and likely is the better path forward. |
|
tyvm both for the clarification and your patience. I will rework the solution to exclusively use build flags.
With runtime checks you can make the current OS a parameter of the |
|
Thanks, A helpful answer. It is indeed true that with the build flags the unit tests cannot cover all systems on one run, but CI provides the checks for those systems - so overall we get the optimised path but get the test runs too. |
ErikKalkoken
force-pushed
the
feature-5108
branch
from
6b914d5
to
4b4edb4
Compare
|
Here is my new attempt ready for review. |
|
Thanks. I had a very quick look through the code on my phone and it looked great. Will follow up with a proper review later in the week when I have more time :) |
Description:
The current implementation renders menu shortcuts exclusively in macOS style, which might be confusing for users on Windows and Linux (see also #5108).
This PR adds a new generic style for rending menu shortcut texts on non-Darwin OS, while keeping the existing macOS style for Darwin OS. The generic style is mostly spelling out the names of keys instead of using symbols and should be much easier to understand for Windows and Linux users. The style is based on the VS Code shortcut documentation for Windows and Linux.
Additional changes
Example screenshot
Fixes #5108
Checklist:
Where applicable:
N/A