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

Minor renamings of some APIs #278

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

Conversation

ethanc8
Copy link
Contributor

@ethanc8 ethanc8 commented Jun 23, 2024

This commit implements Apple's renaming of some APIs, such as another enum NSProgressIndicatorStyle that Apple had renamed in order to allow it to be parsed by the Swift compiler, the new typedef typedef NSString *NSPasteboardType;, and the -[NSViewController view{Will|Did}{Appear|Disappear}] variants which take no arguments.

@ethanc8 ethanc8 requested a review from fredkiefer as a code owner June 23, 2024 19:10
Copy link
Member

@gcasa gcasa left a comment

Choose a reason for hiding this comment

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

It would be best to, instead of renaming them, do a similar version ifdef for the version of macOS this change was made in. We always want to be as backwards compatible as possible. It is okay in an enum for multiple names to be assigned the same value.

@ethanc8
Copy link
Contributor Author

ethanc8 commented Jun 28, 2024

It would be best to, instead of renaming them, do a similar version ifdef for the version of macOS this change was made in. We always want to be as backwards compatible as possible. It is okay in an enum for multiple names to be assigned the same value.

I did not rename any APIs -- I just added the new APIs, and the enums have been defined. I might need to add some guards around the new enum names, though.

@gcasa
Copy link
Member

gcasa commented Jun 28, 2024

It would be best to, instead of renaming them, do a similar version ifdef for the version of macOS this change was made in. We always want to be as backwards compatible as possible. It is okay in an enum for multiple names to be assigned the same value.

I did not rename any APIs -- I just added the new APIs, and the enums have been defined. I might need to add some guards around the new enum names, though.

I apologize, I wasn't clear, this is what I meant. It's so that if someone is using a particular version of the API they don't have access to the enums under the new names.

Copy link
Member

@fredkiefer fredkiefer left a comment

Choose a reason for hiding this comment

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

Looks good

@fredkiefer
Copy link
Member

Could you please add the #ifdef that Gregory requested so we can merge this.

@ethanc8
Copy link
Contributor Author

ethanc8 commented Jun 29, 2024

Could you please add the #ifdef that Gregory requested so we can merge this.

I will do that tomorrow.

@ethanc8
Copy link
Contributor Author

ethanc8 commented Feb 1, 2025

I have rebased it on master and will try updating the patches soon.

@ethanc8
Copy link
Contributor Author

ethanc8 commented Feb 1, 2025

@fredkiefer @gcasa I believe this is ready to merge into master. The CI errors are due to unrelated issues with the workflow YAML definitions.

@@ -83,6 +83,7 @@ - (void)viewDidLoad

- (void) viewWillAppear: (BOOL)animated
{
[self viewWillAppear];
Copy link
Member

Choose a reason for hiding this comment

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

I think the call order should be the other way around. That is in viewWillAppear we should call viewWillAppear: with NO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apple's docs indicate that developers are supposed to override viewWillAppear. viewWillAppear: seems to be a GNUstep extension; it never appears in NSViewController.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if in viewWillAppear we call viewWithAppear:NO people will subclass NSViewController and implement viewWillAppear, but any calls to viewWillAppear:NO in gnustep-gui will bypass their implementation. But if in viewWillAppear: we call viewWithAppear, we will use people's implementations in their subclasses whenever gnustep-gui calls viewWithAppear:NO.

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.

3 participants