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

Implemented a right-click menu for notification icon. #13

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

Eiyeron
Copy link
Contributor

@Eiyeron Eiyeron commented Dec 3, 2024

Hello.

I quickly drafted a right-click menu for the notification icon to quickly have an exit option. I also threw an option to force errored cooking commands as I sometimes get to retrigger those after a few errors.
image

This is more a draft than a serious PR but if you're interested into this PR, I could write a less hacky-looking solution than the one provided here.

Have a nice day.

@jlaumon
Copy link
Owner

jlaumon commented Dec 3, 2024

Cheers, I'll try to take a look tonight.

Out of curiosity, why do you need the cook errored button? Normally you'd have to change an input to fix an error, if you do that it should recook automatically. Is that not working as intended?

@Srekel
Copy link

Srekel commented Dec 3, 2024

Cheers, I'll try to take a look tonight.

Out of curiosity, why do you need the cook errored button? Normally you'd have to change an input to fix an error, if you do that it should recook automatically. Is that not working as intended?

It's quite common for us to need to recook errored a few times to get them all successfully cooked.

@jlaumon
Copy link
Owner

jlaumon commented Dec 3, 2024

Cheers, I'll try to take a look tonight.
Out of curiosity, why do you need the cook errored button? Normally you'd have to change an input to fix an error, if you do that it should recook automatically. Is that not working as intended?

It's quite common for us to need to recook errored a few times to get them all successfully cooked.

Is that because The Forge's Asset Pipeline fails randomly because multiple instances try to open the same log file? We should fix that instead (I have a local change for it haha)

@@ -7,6 +7,11 @@
#include "Strings.h"

constexpr uint32 cNotifCallbackID = 0x8001; // ie. WM_APP + 1
// Not used yet but could be used as an ID mask to dispatch events.
[[maybe_unused]]
constexpr uint32 cNotifMenuId = 0x8100;
Copy link
Owner

Choose a reason for hiding this comment

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

Does that value mean something specific? Or it's just a value that you picked?

Copy link
Contributor Author

@Eiyeron Eiyeron Dec 3, 2024

Choose a reason for hiding this comment

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

I just used 0x81xx as a pseudo-namespace where the least significant byte would be the notification menu's command ID. The upper byte would just identifying the context and masking would be easier to think about.

Just some forward thinking on the spot, not really useful as is for now. Removing the mask for now.

src/main.cpp Outdated

HMENU hMenu = CreatePopupMenu();
gApp.mNotifMenuHmenu = hMenu;
ret = InsertMenu(hMenu, 0, MF_BYPOSITION | MF_STRING, cNotifMenuCookErrored, L"Cook errored");
Copy link
Owner

@jlaumon jlaumon Dec 3, 2024

Choose a reason for hiding this comment

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

In the rest of the code I've tried to use the A version of the Windows functions instead of the WCHAR ones. They should support receiving utf-8 strings thanks to the enable_utf8.manifest (but does need checking, because some of then don't work!)

Just try InsertMenuA(..., (const char*)u8"Cook errored 😭") and see what happens :D

Copy link
Contributor Author

@Eiyeron Eiyeron Dec 3, 2024

Choose a reason for hiding this comment

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

My bad, I noticed usage of W functions in the same file so I went with the flow. I'm going to change them then, thanks for pointing at it.

@Eiyeron
Copy link
Contributor Author

Eiyeron commented Dec 3, 2024

As discussed on the Fediverse instead of here due to an issue accessing GitHub I also removed the test command and added a pause cooking button instead.

Addendum:

Out of curiosity, why do you need the cook errored button? Normally you'd have to change an input to fix an error, if you do that it should recook automatically. Is that not working as intended?

There is an issue, semi-related to Srekel's own report, that triggered the idea: as I'm working on shaders with AssetCooker, I might save the same file in an erroneous state few times in a row. Sometimes AssetCooker seems to give up on that file and I need to forcefully acknowledge to restart its cooking in order to get it chopping. I chalked that on an automatic disablement of a possibly broken rule and just wrote the button as a shortcut, thinking it'd be akin to a fail state flush and reset.

@jlaumon jlaumon marked this pull request as ready for review December 4, 2024 08:08
@jlaumon
Copy link
Owner

jlaumon commented Dec 4, 2024

Looks good to me! I'll give it a try tonight and merge if I see no issues 👍

@jlaumon jlaumon merged commit 99d4b7e into jlaumon:main Dec 5, 2024
1 check passed
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.

3 participants