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

Deprecation cleanup #2075

Merged
merged 17 commits into from Oct 30, 2015
Merged

Deprecation cleanup #2075

merged 17 commits into from Oct 30, 2015

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Jul 9, 2015

Here's an attempt at bringing down the count of warnings. I even tried to kill the ND classes ;-).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 29, 2015

Nice looking cleanup here. How did you update CGSPrivate? Did you manually do it yourself, or copy/paste from an updated copy online?
I've tried to do it countless times in the past, but it's always caused crashes (e.g. with things like the cube animation effect etc.)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 29, 2015

e.g. with things like the cube animation effect etc.

Actually, since we've killed the cube interface and we've removed the effect from the setup screen it shouldn't be a problem. I'm running with this now

];
NSURL *targetURL = [NSURL URLByResolvingBookmarkAtURL:currentURL options:NSURLBookmarkResolutionWithoutUI bookmarkDataIsStale:NULL error:&err];
if (!targetURL) {
NSLog(@"Error resolving alias: %@", err);
Copy link
Member

@skurfer skurfer Aug 6, 2015

Choose a reason for hiding this comment

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

I’m seeing this logged hundreds of times. Most of them say because the thing is a directory, but testing NSURLIsDirectoryKey first doesn’t seem to return YES so I’m not sure what the issue is.

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 6, 2015

Besides the comment on the diff, I also saw at least one double free after running this for a while. I didn’t have time to find a way to reproduce, but if I do, I’ll let you know.

@tiennou tiennou mentioned this pull request Aug 6, 2015
@tiennou
Copy link
Member Author

@tiennou tiennou commented Aug 6, 2015

@pjrobertson I copied the "new" BSD-licenced version and readded the missing things on top of it.

@skurfer I'll take another look at that. To be frank, I was trying to kill NDAlias because of the deprecation warnings so I didn't run much. I'm also concerned that, since aliases are (to me) opaque OS X things, Apple could have changed something between the old Carbon API (used by NDAlias) and the new NSURL bookmarks. And then, maybe thinking some more, maybe those pre-OS X days are sufficiently behind us that we shouldn't care too much ;-).

@tiennou tiennou force-pushed the t/deprecation-cleanup branch 2 times, most recently from 70dc029 to 6be3a85 Compare Sep 1, 2015
@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 1, 2015

Rebased against master, without the switch-to-NSURL-bookmarks thing in because @skurfer said so (sorry your comment got lost since I rebased...EDIT looooks like it didn't, which is a nice thing). We'll change to bookmarks later, so at least this can clear the warning queue a little...

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 2, 2015

without the switch-to-NSURL-bookmarks thing

Remind me, do you mean the removal of NDAlias and stuff?

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 2, 2015

That's something else, but those NSURL changes are required before we can drop NDAlias. Agreed, those are just one-step methods around the 2-step API (make bookmark+write data / read data+resolve bookmark). I haven't been able to see the hundreds of logs @skurfer reports seeing, but I don't have that many aliases around.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 7, 2015

I was trying to test a bunch of stuff at once, and I noticed this conflicts with your changes in https://github.com/quicksilver/Quicksilver/tree/t/notification-rewrite

Do you want to just include those changes here so we don’t run into the conflict later?

@tiennou tiennou force-pushed the t/deprecation-cleanup branch from 6be3a85 to c72a435 Compare Oct 27, 2015
@tiennou
Copy link
Member Author

@tiennou tiennou commented Oct 27, 2015

Just rebased this one and the notification-rewrite thing on top so there should be no conflicts.

@tiennou tiennou mentioned this pull request Oct 28, 2015
@skurfer
Copy link
Member

@skurfer skurfer commented Oct 29, 2015

Just rebased this one and the notification-rewrite thing on top so there should be no conflicts.

Thanks! I think I’ve gone over this a couple of times already, so I’ll give it another quick look and probably merge tomorrow. Then it’s on to the aliases.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Oct 29, 2015

That would be good. I'm basing most of my other work on this one, so if it can go in quickly without too much regressions, I'd be happy ;-).

skurfer added a commit that referenced this issue Oct 30, 2015
@skurfer skurfer merged commit c88526f into master Oct 30, 2015
2 checks passed
skurfer added a commit that referenced this issue Oct 30, 2015
@skurfer
Copy link
Member

@skurfer skurfer commented Oct 30, 2015

One down!

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.

None yet

3 participants