Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nasehim7
Copy link
Contributor

@nasehim7 nasehim7 commented Mar 4, 2024

Resolves: #18014

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

Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For testing purposes, can you add the icon to both the isCloud and else cases, but do so as follows:

  • Cloud: ☁️ \u2601\uFE0F (emoji version)
  • Not cloud: ☁︎ \u2601\uFE0E (text version)

This makes use of \uFE0E (text selector) and \uFE0F (emoji selector), which determine how the preceding character is interpreted.

Adding both selectors ensures we get to see both symbols, so we can pick the one that looks best (or neither). Thanks!

src/appshell/view/appmenumodel.cpp Outdated Show resolved Hide resolved
@nasehim7
Copy link
Contributor Author

nasehim7 commented Mar 8, 2024

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

@shoogle
Copy link
Contributor

shoogle commented Mar 8, 2024

@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.

@nasehim7
Copy link
Contributor Author

nasehim7 commented Mar 8, 2024

I see, on MacOS it looks like below

Screenshot 2024-03-08 at 9 06 54 PM

@shoogle
Copy link
Contributor

shoogle commented Mar 8, 2024

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
Copy link
Contributor

shoogle commented Mar 8, 2024

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

@nasehim7
Copy link
Contributor Author

nasehim7 commented Mar 8, 2024

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

@cbjeukendrup
Copy link
Contributor

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.

@shoogle
Copy link
Contributor

shoogle commented Mar 8, 2024

@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.

@nasehim7
Copy link
Contributor Author

nasehim7 commented Mar 8, 2024

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

@nasehim7
Copy link
Contributor Author

nasehim7 commented Mar 8, 2024

@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?

@shoogle
Copy link
Contributor

shoogle commented Mar 9, 2024

@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));
}

@nasehim7
Copy link
Contributor Author

nasehim7 commented Mar 9, 2024

@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 nasehim7 force-pushed the fixCloudIcon branch 5 times, most recently from 613500e to 5344f31 Compare March 9, 2024 09:48
@cbjeukendrup
Copy link
Contributor

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.

@shoogle
Copy link
Contributor

shoogle commented Mar 10, 2024

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.

@cbjeukendrup
Copy link
Contributor

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.

@jessjwilliamson
Copy link
Contributor

jessjwilliamson commented Mar 11, 2024

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.

@cbjeukendrup
Copy link
Contributor

@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.

@jessjwilliamson
Copy link
Contributor

@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
Copy link
Contributor

Cloud icon.zip

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

@nasehim7
Copy link
Contributor Author

nasehim7 commented Mar 11, 2024

@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

@cbjeukendrup
Copy link
Contributor

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.

@@ -25,6 +25,7 @@ if (OS_IS_MAC)

install(FILES MsczIcon/MS4_MsczIcon.icns RENAME MsczIcon.icns DESTINATION ${Mscore_SHARE_NAME}${Mscore_INSTALL_NAME})
install(FILES MscxIcon/MS4_MscxIcon.icns RENAME MscxIcon.icns DESTINATION ${Mscore_SHARE_NAME}${Mscore_INSTALL_NAME})
install(FILES cloud_icon.svg DESTINATION ${Mscore_SHARE_NAME}${Mscore_INSTALL_NAME})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should move these install commands out of the platform if statements and only write it once since it's the same on all platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah makes sense. Updated the PR.

@RomanPudashkin
Copy link
Contributor

@nasehim7 please rebase

@@ -74,6 +74,8 @@ class MenuItem : public QObject, public async::Asyncable

Q_PROPERTY(QList<MenuItem*> subitems READ subitems NOTIFY subitemsChanged)

Q_PROPERTY(QString nativeMenuBarIconPath READ nativeMenuBarIconPath NOTIFY actionChanged)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you're not using nativeMenuBarIconPath

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the changes to PlatformMenuBar.qml got lost

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this gets merged, I think either the PNG or the SVG should be removed, depending on what works (where SVG has preference, if both work).

@@ -149,6 +154,8 @@ public slots:
QList<MenuItem*> m_subitems;

ui::UiAction m_action;

QString m_nativeMenuBarIconPath = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
QString m_nativeMenuBarIconPath = "";
QString m_nativeMenuBarIconPath;

("" is not the same as QString(); the latter is more efficient.)

@@ -112,5 +112,6 @@
<file>qml/Preferences/internal/MixerSection.qml</file>
<file>qml/DevTools/Extensions/ExtensionsListView.qml</file>
<file>qml/platform/PlatformMenuBar.qml</file>
<file>resources/cloud_icon.svg</file>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is also still some redundancy that should be resolved; either this one should be used or the "installed" one

@@ -74,6 +74,8 @@ class MenuItem : public QObject, public async::Asyncable

Q_PROPERTY(QList<MenuItem*> subitems READ subitems NOTIFY subitemsChanged)

Q_PROPERTY(QString nativeMenuBarIconPath READ nativeMenuBarIconPath NOTIFY actionChanged)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the changes to PlatformMenuBar.qml got lost

@nasehim7
Copy link
Contributor Author

@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 nasehim7 force-pushed the fixCloudIcon branch 2 times, most recently from d455498 to fe15ee3 Compare April 19, 2024 20:18
@nasehim7
Copy link
Contributor Author

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

@shoogle
Copy link
Contributor

shoogle commented Apr 20, 2024

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.

@cbjeukendrup
Copy link
Contributor

We can try, but I doubt that ICNS will make any difference. It will namely go through QIcon anyway, and I don't know if that even supports ICNS.

Another approach could be to create a minimal test app that creates a MenuBar in the more usual way, namely just writing the menus out like this: https://doc.qt.io/qt-6/qml-qt-labs-platform-menubar.html#details
And try to get it work that way.

If that works, then we are somehow doing something wrong in the way we create our menus; if it doesn't work, then the problem is in QML. That way, we'll at least get an idea of where the problem lies.

Qt Creator has some templates to set up a simple QML app with a few clicks, so fortunately that's not a huge amount of work.

@nasehim7
Copy link
Contributor Author

Icon Testing Summary:

Tested AppIcon.icns in the main app: No success.

Created Simple QML Test App: View here
Results with the Test App:
Using QRC: Icon did not display.
Using Absolute Path (file:https:///): Icon displayed correctly.

Observation: In the PR, I realised I haven't used file:https:/// prefix while setting nativeMenuBarIconPath, which is necessary. However, fixing it did not resolve the issue in the main app. This brings me to the conclusion that the menu creation piece might need a change, still on it

@cbjeukendrup
Copy link
Contributor

I did get it to work with QRC in this small example project:
test-3.zip

So it seems not generally allergic to QRC, but we are doing something wrong.

@cbjeukendrup
Copy link
Contributor

For tonight, my conclusion is that icons don't work when the MenuItem is created using a Component.
In the following code:

ApplicationWindow {
    visible: true
    width: 640
    height: 480
    title: "QML MenuBar Example"

    MenuBar {
        Menu {
            title: "File"
            MenuItem {
                text: "Open"
                icon.source: "qrc:/icons/cloud_icon.svg"
                icon.mask: true
            }
            MenuItem { text: "Close" }
            id: fileMenu
        }
        Menu {
            title: "Edit"
            MenuItem { text: "Undo" }
            MenuItem { text: "Redo" }
        }
    }

    Component.onCompleted: {
        let menuItem = menuItemComponent.createObject(fileMenu)
        fileMenu.insertItem(1, menuItem)
    }

    Button {
        icon.source: "qrc:/icons/cloud_icon.svg"
    }

    Component {
        id: menuItemComponent

        MenuItem {
            text: "New"
            icon.source: "qrc:/icons/cloud_icon.svg"
            icon.mask: true
        }
    }
}

the "Open" item does have an icon, and the "New" item doesn't.

@nasehim7
Copy link
Contributor Author

Yes the example works perfectly. Strangely, I am curious on why the qrc wasn't working in my case but yeah for now, let me update the menu item construction in the main qml and check

@nasehim7
Copy link
Contributor Author

nasehim7 commented Apr 27, 2024

Hi @shoogle @cbjeukendrup, I was tied up with college work recently and couldn't look into this until today. Upon revisiting and testing the code, I noticed a behaviour: the icon loads some time after the application is run, which suggests that the current menu item creation is still functional. For reference, I am attaching two screen recordings.

First
In this video, before opening a score, I opened recents and there was no cloud icon in the menu items but when I opened a score and then opened the recents again, cloud icon was visible.

Second
In this one, it can be seen that the icon is rendered after a decent while.

I found about the asynchronous nature of how image loading works as given here. I couldn't find something specific to Icons. However, in the documentation it says, by default elements loaded remotely are async in nature and for the local file, we have to especially make them async and we haven't done that.

P.S. I tried with both svg and png, it works. In the video, it is svg version.

@cbjeukendrup
Copy link
Contributor

That's a surprising discovery. That documentation mentioning the asynchronous image loading seems to apply to the Image component, but that's not being used here. Some time ago, I studied the related Qt source code a bit, and it looks like there's a QIcon being created in C++ and that's then converted to a macOS-native icon in the platform-specific Qt code where the native menu items are being created. I didn't see anything asynchronous, but might have missed something.

The thing to find out now is: is there really something asynchronous going on, or is there some trigger that causes the icon to appear at some point? And can we artificially create that trigger directly when creating the menu so that the icon is always there when it should be? Perhaps the only way to find out is by debugging Qt itself, but I don't really know how to do that.

I'm not really sure how to move forward with this issue. @shoogle what do you think?

@shoogle
Copy link
Contributor

shoogle commented Apr 29, 2024

Perhaps the trigger is when a cloud score changes position in the Open Recent list? For example:

  • If you open a cloud score then it would go to the top of the Recent list and the cloud icon will appear.
  • If you open a local score that's older than a cloud score, it would bump the cloud score 1 place further down the list, so the icon will appear.
  • However, if you open a score that's more recent than the latest cloud score then the position of the cloud score won't change, so the icon won't appear.

I haven't tried it myself, but this is consistent with what we see in @nasehim7's screen recordings. Perhaps artificially shuffling the 1st item to the end of the menu and back again would be enough to trigger the cloud icon for all menu items that need it.

I don't think we can merge this with it only half working on macOS. That's probably worse than it not working at all, so either we fix it properly on macOS, or we abandon macOS and just implement the icon on Windows and Linux.

It occurred to me that we could also hide the file extension for cloud scores, so there would still be a way to tell if it's a cloud score even if the icon doesn't appear.

@nasehim7
Copy link
Contributor Author

nasehim7 commented Apr 29, 2024

@shoogle Indeed, I tested for few scenarios and could see that the icon visibility is linked with position change of the cloud score. If I keep the recent score as the cloud one, close the application and run it again. Until the position of the score is not changed, the icon is not visible. Till the time I am looking at the shuffling option, the second option you suggested should be straight forward to implement by just sending false to the displayName when setting the action.title. Do we need to prioritise the second approach for now or should we first explore the shuffling option?

@shoogle
Copy link
Contributor

shoogle commented Apr 29, 2024

@nasehim7, let's keep this PR for macOS. Feel free to try the shuffling option. You could also try deleting the menu and recreating it only once all the items inside it have been initialised.

You could create a new PR with the changes for other platforms, including hiding the file extension for cloud scores.

@shoogle
Copy link
Contributor

shoogle commented Apr 29, 2024

Shuffling won't work if there's only one item in the menu, so a better solution is deleting the menu and recreating it like I mentioned above, or adding a dummy item to the beginning of the menu and then removing that item.

@cbjeukendrup
Copy link
Contributor

@nasehim7 Could you please remind me, did you indeed create a new PR with only the non-macOS-specific changes from this PR? (i.e. basically only the action.iconCode = IconCode::Code::CLOUD; part)

@nasehim7
Copy link
Contributor Author

@cbjeukendrup No I haven't yet. I will do it this week and update here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloud scores don't have labels (marks) in File > Open recent dropdown
7 participants