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 deprecated methods. Fixes #648 #781

Merged
merged 2 commits into from Apr 7, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 30, 2012

I've just removed the deprecated method all together.

The reason there is no warning in the console in 10.7 is because the deprecated -(void)selectRow:(int)row byExtendingSelection:(BOOL)extend method has been dropped, so it never gets called.
Andreberg added in the new method some time ago as a 'fall back', which means that everything works on Lion just fine.

It appears 10.6 is still calling the old deprecated method, and as such it gives the warning. This change is untested on 10.6, so I do not know if this will cause a crash.

@HenningJ - could you perform the following:

  • Merge this and build Quicksilver
  • Add 2 breakpoints to QSFancyTableView.m:45 and QSFancyTableView.m:101
  • Open up Quicksilver's preference and go to the triggers/catalog/preferences panes
  • Test to see if the breakpoints are triggered. In which case 10.6 is using the new method fine
  • Make sure you get no crashes and things work properly. If 10.6 doesn't call the new method, then loading QSFancyTables won't call the [self setNeedsDisplay:YES]; part of the method

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 30, 2012

Since this is clean-up related, would you be willing to add another commit that removes the (non-existent) QSSoundPlayAction from the core plug-in’s plist? I ran across it when looking at #774. This is what’s causing the repeated “Failed loading bundle (null)” message. Actually, I’m not sure if any of the references to QSSound* are still valid.

Seemed like a silly thing to have its own pull request, but if you prefer, I can create one.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 30, 2012

done.

I've also removed it from the .strings files.

On 30 March 2012 15:42, Rob McBroom <
reply@reply.github.com

wrote:

Since this is clean-up related, would you be willing to add another commit
that removes the (non-existent) QSSoundPlayAction from the core plug-ins
plist? I ran across it when looking at #774. This is whats causing the
repeated Failed loading bundle (null) message. Actually, Im not sure if
any of the references to QSSound* are still valid.

Seemed like a silly thing to have its own pull request, but if you prefer,
I can create one.


Reply to this email directly or view it on GitHub:
#781 (comment)

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 2, 2012

I have no issues with this, but like you said, it might be good to get a 10.6 user to test.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 6, 2012

I just tried it. The breakpoint at QSFancyTableView.m:45 is triggered fine, the one on line 101 isn't. When exactly should that happen? Otherwise it seems fine.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 6, 2012

I can't remember exactly, but I think link 101 is either for the
shelf/clipboard or one of the prefs views.

As long as 10.6 is using the newer - (void)selectRowIndexes:(NSIndexSet *) indexes byExtendingSelection:(BOOL)extend; method by default, and isn't
complaining that the other method doesn't exist I think it's all fine.

Good stuff, thanks Henning

On 6 April 2012 16:26, Henning Jungkurth <
reply@reply.github.com

wrote:

I just tried it. The breakpoint at QSFancyTableView.m:45 is triggered
fine, the one on line 101 isn't. When exactly should that happen? Otherwise
it seems fine.


Reply to this email directly or view it on GitHub:
#781 (comment)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 7, 2012

It seems to work fine, so it's good to go, right?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 7, 2012

Yep :)

On 7 April 2012 14:57, Henning Jungkurth <
reply@reply.github.com

wrote:

It seems to work fine, so it's good to go, right?


Reply to this email directly or view it on GitHub:
#781 (comment)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 7, 2012

I've merged

pjrobertson added a commit that referenced this issue Apr 7, 2012
Remove deprecated methods. Fixes #648
@pjrobertson pjrobertson merged commit e07a8fc into quicksilver:master Apr 7, 2012
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