MuseScore icon indicating copy to clipboard operation
MuseScore copied to clipboard

Fix #18014: Cloud score missing label in open recent menu

Open nasehim7 opened this issue 11 months ago • 41 comments

Resolves: #18014

  • [x] I signed the CLA
  • [x] The title of the PR describes the problem it addresses
  • [x] Each commit's message describes its purpose and effects, and references the issue it resolves
  • [x] If changes are extensive, there is a sequence of easily reviewable commits
  • [x] The code in the PR follows the coding rules
  • [x] There are no unnecessary changes
  • [x] The code compiles and runs on my machine, preferably after each commit individually
  • [x] I created a unit test or vtest to verify the changes I made (if applicable)

nasehim7 avatar Mar 04 '24 14:03 nasehim7

@shoogle Hi Peter, updated the PR with the requested changes

nasehim7 avatar Mar 08 '24 19:03 nasehim7

@nasehim7, thanks! I'm not seeing any difference on Windows, but that could just mean that the menu font doesn't have a glyph for the emoji so it falls back to using the text character glyph instead.

image

It would be interesting to see what these look like on other platforms.

shoogle avatar Mar 08 '24 21:03 shoogle

I see, on MacOS it looks like below

Screenshot 2024-03-08 at 9 06 54 PM

nasehim7 avatar Mar 08 '24 21:03 nasehim7

Ok, thanks! The emoji on macOS looks much better, which good because we can't control the fonts in the macOS menus.

On Windows, the cloud looks rubbish, but we control the fonts in the menus on Windows, so we could use a different font for the icon, or modify the current font to include the emoji glyph.

shoogle avatar Mar 08 '24 21:03 shoogle

Could you take a screenshot of the menus in macOS Light Theme?

shoogle avatar Mar 08 '24 21:03 shoogle

Ah I see. So if we decide on the font change for windows that change should also go in with this PR, right?

Yes, here is the light theme screenshot:

Screenshot 2024-03-08 at 9 34 37 PM

nasehim7 avatar Mar 08 '24 21:03 nasehim7

On non-Mac, why not just using the icon from the icon font, by setting the iconCode of the action? The solution with unicode characters was only invented as a workaround for macOS, which uses the native menu bar and it's too much effort to show icons from the icon font there.

cbjeukendrup avatar Mar 08 '24 21:03 cbjeukendrup

@cbjeukendrup, good point!

Can someone with macOS take a screenshot of the emoji when the menu text is dark, like in this one. You might need to put MuseScore in its dark theme or change your desktop background in order to change the menu text colour.

shoogle avatar Mar 08 '24 21:03 shoogle

Here it is Screenshot 2024-03-08 at 10 19 29 PM I suspect It is inconsistent due to the variant selector

nasehim7 avatar Mar 08 '24 22:03 nasehim7

@cbjeukendrup What you suggested would loosely boil down to something like this:

if (isCloud) {
    #if defined(Q_OS_WIN) || defined(Q_OS_LINUX)
    action.iconCode = IconCode::Code::CLOUD;
    #elif defined(Q_OS_MAC)
    action.title = TranslatableString::untranslatable("\u2601\uFE0F " + file.displayName(/*includingExtension*/ true));
    #endif
}

is it?

nasehim7 avatar Mar 08 '24 22:03 nasehim7

@nasehim7 it's the right approach but no need to detect Windows and Linux explicitly. You can do #if defined(Q_OS_MAC) ... #else ... #endif. And you'll still need to set the action.title on non-macOS platforms, just exclude the emoji character.

I suspect It is inconsistent due to the variant selector

Adobe was able to make the nicely shaped cloud black when the text is black, so it looks like we still haven't found the right approach yet for macOS.

Please add all of these characters with the current code layout so we can see how these look in the macOS menu:

\u2601 cloudy weather 🌧\u1F327 cloud with rain ⛅\u26C5 sun behind cloud ⛈\u26C8 thunder cloud 🌩\u1F329 cloud with lightning

Also add the line action.iconCode = IconCode::Code::CLOUD; so we can compare to our own cloud icon. It won't do anything on macOS but will affect the other platforms.

if (isCloud) {
    action.iconCode = IconCode::Code::CLOUD;
    action.title = TranslatableString::untranslatable("\u2601\uFE0F\u1F327\uFE0F\u26C5\uFE0F\u26C8\uFE0F\u1F329\uFE0F " + file.displayName(/*includingExtension*/ true));
} else {
    action.title = TranslatableString::untranslatable("\u2601\uFE0E\u1F327\uFE0E\u26C5\uFE0E\u26C8\uFE0E\u1F329\uFE0E " + file.displayName(/*includingExtension*/ true));
}

shoogle avatar Mar 09 '24 00:03 shoogle

@shoogle Yes that makes sense. I updated the code and checked the output, 2nd and 5th text versions are not getting rendered properly. Here is the screenshot:

Screenshot 2024-03-09 at 8 01 51 AM

P.S.: while we are finalising for MacOS, I added completed code for other platforms so that they can be tested as well.

nasehim7 avatar Mar 09 '24 08:03 nasehim7

Adobe was able to make the nicely shaped cloud black when the text is black, so it looks like we still haven't found the right approach yet for macOS.

Adobe probably doesn't use a text character, but uses the proper macOS APIs to add an icon to a menu item. Either they use these APIs directly, or they use them via whatever UI framework they use.

It seems we could also achieve that using the icon property of QML's MenuItem: https://doc.qt.io/qt-6.2/qml-qt-labs-platform-menuitem.html#icon-prop But that's still not trivial, because QML uses a very different format for icons than we do. In MuseScore, all icons are actually characters from a font, and can be accessed using the IconCode enum. But Qt expects a URL where a SVG or PNG or so can be found. I'm not sure yet if there is an elegant way around that. The non-elegant way would be to put a SVG version of all icons in some folder, but that's basically out of question.

What may or may not make the challenge bigger is that on macOS we would only want cloud icons for the 'open recent' list, and not all action icons that can be seen in the menus on Windows and Linux.

cbjeukendrup avatar Mar 10 '24 00:03 cbjeukendrup

It sounds like we just need an SVG version of the cloud icon, or has someone requested that we show other icons too? If they have, there are tools that can convert fonts to SVG (and then to PNG if necessary), but we should probably verify the method works with the cloud SVG first before we go about converting an entire font.

shoogle avatar Mar 10 '24 13:03 shoogle

For now, it has indeed only been requested for the cloud icons. But having exceptions does not necessarily make things easier. Anyway, afaics, the most straightforward solution would be to add yet another field to MenuItem, namely nativeMenuBarIconPath, which we'll use in this situation only.

cbjeukendrup avatar Mar 10 '24 14:03 cbjeukendrup

Adobe was able to make the nicely shaped cloud black when the text is black, so it looks like we still haven't found the right approach yet for macOS.

Adobe probably doesn't use a text character, but uses the proper macOS APIs to add an icon to a menu item. Either they use these APIs directly, or they use them via whatever UI framework they use.

It seems we could also achieve that using the icon property of QML's MenuItem: https://doc.qt.io/qt-6.2/qml-qt-labs-platform-menuitem.html#icon-prop But that's still not trivial, because QML uses a very different format for icons than we do. In MuseScore, all icons are actually characters from a font, and can be accessed using the IconCode enum. But Qt expects a URL where a SVG or PNG or so can be found. I'm not sure yet if there is an elegant way around that. The non-elegant way would be to put a SVG version of all icons in some folder, but that's basically out of question.

What may or may not make the challenge bigger is that on macOS we would only want cloud icons for the 'open recent' list, and not all action icons that can be seen in the menus on Windows and Linux.

Incase you didn't see i commented about Adobe using the same icon on Windows version. If we need a SVG for a cloud icon for MS4, lemme know, I am happy to provide easily.

jessjwilliamson avatar Mar 11 '24 15:03 jessjwilliamson

@jessjwilliamson On Windows we would indeed use the same icon but there it can be done trivially via the icons font, because there we have our custom menu bar. In order to get the icon into the macOS menu bar too (and also into the native menubar on Linux OSs that support it), it looks indeed like having an SVG version would be great.

cbjeukendrup avatar Mar 11 '24 16:03 cbjeukendrup

@jessjwilliamson On Windows we would indeed use the same icon but there it can be done trivially via the icons font, because there we have our custom menu bar. In order to get the icon into the macOS menu bar too (and also into the native menubar on Linux OSs that support it), it looks indeed like having an SVG version would be great.

That's not a problem, I'll get a SVG done soon :)

jessjwilliamson avatar Mar 11 '24 16:03 jessjwilliamson

Cloud icon.zip

I thought i had to design one but there's a perfect one in our font. Just made it an SVG.

jessjwilliamson avatar Mar 11 '24 16:03 jessjwilliamson

@cbjeukendrup Here I am summarising the potential approach that can be taken to fix this:

  1. Add nativeMenuBarIconPath member to MenuItem with default value as empty String. Add it's getter, setters.
  2. We can put the cloud_icon.svg in appshell -> qrc:/qml/resources/cloud_icon.svg (needs confirmation)
  3. Need to add resource path to appshell.qrc, I suppose.
  4. In the appmenumodel.cpp we should do something like this -
if (isCloud) {
     #ifdef Q_OS_MAC
     item->setNativeMenuBarIconPath("qrc:/qml/resources/cloud_icon.svg");
     #else
     action.iconCode = IconCode::Code::CLOUD;
     #endif
}

the action.title will set afterwards 5. In PlatformMenuBar.qml of MacOS, we will update the makeMenuItem() method to have - if (itemInfo.nativeMenuBarIconPath !== "") { menuItem.icon.source = itemInfo.nativeMenuBarIconPath; }

let me know if I missed on something crucial

nasehim7 avatar Mar 11 '24 23:03 nasehim7

Sounds good! A few more thoughts:

  • I'd put the icon in qrc:/resources/cloud_icon.svg (i.e. drop /qml from the path)
  • You could remove the #ifdef Q_OS_MAC/#else checks, and just let both lines run on all OSs. This way, Linux platforms that have a systemwide menubar (i.e. similar to macOS) will also benefit.
  • In order for light/dark mode to work properly, you might need to set menuItem.icon.mask to true.

cbjeukendrup avatar Mar 12 '24 01:03 cbjeukendrup

I did the discussed approach + changes but the cloud_icon.svg is not being rendered on the UI. I did console.log() in the QML to check if the nativeMenuBarIconPath being set correctly and it is indeed qrc:/resources/cloud_icon.svg whenever I open the menu. I double checked at the file path and also the path has been added in the appshell.qrc file, they seem correct. Any thoughts? I am using Qt 5.15 so it supports the SVG icon source.

nasehim7 avatar Mar 12 '24 13:03 nasehim7

@nasehim7, can you push the new code? The Unicode characters aren't going to work unfortunately, so no need to keep that code.

shoogle avatar Mar 12 '24 15:03 shoogle

Also, here's a PNG version of the icon you can use, just to try to see something in the menu: Cloud_icon

shoogle avatar Mar 12 '24 15:03 shoogle

@shoogle yes sure! let me try and get back

nasehim7 avatar Mar 12 '24 15:03 nasehim7

@shoogle I tried to incorporate the png but still a challenge in rendering it on the UI. I have updated the code as well here for any reference, incase I am missing anything from my side

nasehim7 avatar Mar 12 '24 21:03 nasehim7

@nasehim7 please rebase

RomanPudashkin avatar Apr 19 '24 06:04 RomanPudashkin

@cbjeukendrup @Eism I tried to check it with both the svg and png and it didn't work on the Mac. I think this PR was still open due to that. I will check/test it again today and get back with the results and suggested changes.

nasehim7 avatar Apr 19 '24 14:04 nasehim7

Restored PlatformMenuBar.qml. Currently, kept only the second approach discussed on this thread, installing the assets and using them. The icons still aren’t showing up when I set the icon.source. I also tried the first method, adding the file to appshell.qrc and using the direct path, but no luck there either. I explored and got to know .icns format is compatible with macOS. Also, the appicon is in .icns format. Should we try converting the SVG to ICNS to see if it resolves the issue? Would that be a good way to go? I’m all ears for any suggestions or ideas

nasehim7 avatar Apr 19 '24 20:04 nasehim7

Should we try converting the SVG to ICNS to see if it resolves the issue?

That's a good idea, but before you do that, try displaying the app icon in the menu. The app icon is already ICNS.

shoogle avatar Apr 20 '24 14:04 shoogle