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

Remove GSFakeNSMenuItem class #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

williameveretteggplant
Copy link

This class doesn't appear to do anything but serve as a pass-through for the original object, so why not just use that?

This class doesn't appear to do anything but serve as a pass-through for the original object, so why not just use that?
@williameveretteggplant williameveretteggplant changed the title Draft: Remove GSFakeNSMenuItem class Remove GSFakeNSMenuItem class Oct 25, 2024
@@ -151,7 +71,7 @@ void initialize_lock()
}

// find all subitems for the given items...
HMENU r_build_menu_for_itemmap(NSMenu *menu, BOOL asPopUp, BOOL fakeItem, NSMapTable *itemMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a great idea to rename the variable fakeItem to notPullDown (or something similar) as this value is initially set to ![cell pullsDown]in displayPopUpMenu (line 554). But I'd suggest we update the name throughout, i.e. rename the fake variable in displayPopUpMenu and any other method that may use that name?

@qmfrederik
Copy link
Contributor

Left a comment, LGTM otherwise. @rfm @fredkiefer @gcasa - any thoughts?

@fredkiefer
Copy link
Member

I am not familiar with this theme. If popup menus still work with your change, this sounds like a simplification that we should use. But @gcasa should decide.

@gcasa
Copy link
Member

gcasa commented Oct 26, 2024

I agree with @fredkiefer. Please test pop up menus. If that works then the change is acceptable

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

Successfully merging this pull request may close these issues.

4 participants