Add menu shortcut texts for non-darwin OS

[ErikKalkoken]

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

• Refactored existing code
• Added unit tests for function that generates shortcut texts

Example screenshot

Fixes #5108

Checklist:

• [x] Tests included.
• [x] Lint and formatter run with no errors.
• [x] Tests all pass.

Where applicable:

N/A

[coveralls]

coverage: 59.962% (+0.004%) from 59.958% when pulling 6b914d530af5a9c28b172f7c06c35df21b97328c on ErikKalkoken:feature-5108 into 85063d51a54c2e47df798cb853f02701c59862c2 on fyne-io:develop.

[andydotxyz]

Yes a runtime check might seem simpler (not sure how using runtime checks is easier to unit test TBH). But for efficiency the build flags are preferable because there is less code to compile and the compiler can eliminate or inline the static const/var declarations etc. I don't think that Jacob was asking for the "2) hybrid" I would have thought that using A) for all of them matches the current code style most effectively.

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.

[Jacalz]

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.

[ErikKalkoken]

tyvm both for the clarification and your patience. I will rework the solution to exclusively use build flags.

@andydotxyz

Yes a runtime check might seem simpler (not sure how using runtime checks is easier to unit test TBH).

With runtime checks you can make the current OS a parameter of the textsForShortcuts() function and then test all OS cases with unit tests (see also my initial commit for an example). This works here, because the "platform specific" code is just some different texts and formatting and runs on all platforms. But I fully understand your point about consistency; this was just to answer your question.

[andydotxyz]

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. You'll find that on this path some tests /have/ to run on the target system (such as visual confirmation that symbols render in menus) - so the infrastructure is already set up that way.

[ErikKalkoken]

Here is my new attempt ready for review.

[Jacalz]

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 :)