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

(10.7+) New Plugin updater. Fixes #1332 #1354

Merged
merged 18 commits into from Apr 23, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 27, 2013

This is seriously cool. and fixes #1332 ;-)

It adds a new modal window for installing plugins with the following features:

  • Pick and choose which plugins you want to update (good for e.g. when some users hadn't updated to the latest iTunes and still wanted to use the plugin)
  • View changes
  • View old/new version number
  • Fancy sliding/resizing (OK, that's not a feature) :P
  • scope for much more

It could probably do with a bit of tidying up round the edges, so I thought I'd open a pull for discussion.

To test this out, you need to have some 'old' plugins on your system that you can update. Here's a script to install old versions of Safari, Terminal and Contacts (assuming you have the latest). Run it then restart QS and click 'check now' for updates:

#!/bin/sh

cd "$HOME/Library/Application Support/Quicksilver/PlugIns/"
# remove new plugins
for PLUGIN in "com.blacktree.Quicksilver.QSSafariPlugIn.137.qsplugin" "com.blacktree.Quicksilver.QSTerminalPlugIn.139.qsplugin" "com.blacktree.Quicksilver.QSAddressBookPlugIn.171.qsplugin"
do
    rm -rf $PLUGIN
done
# get older plugins
for PLUGIN in "com.blacktree.Quicksilver.QSSafariPlugIn__310.qspkg" "com.blacktree.Quicksilver.QSTerminalPlugIn__295.qspkg" "com.blacktree.Quicksilver.QSAddressBookPlugIn__366.qspkg"
do
    wget "http://qsapp.com/qs0/plugins/files/$PLUGIN"
    ditto -x -rsrc $PLUGIN .
    rm $PLUGIN
done

Meh, it wasn't looking that sexy so I've added a bit of eye candy :)

Screen Shot 2013-01-27 at 21 29 05

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 27, 2013

Quick question: should the window have a title bar?
Screen Shot 2013-01-27 at 21 34 29

I think it should, so I've added it in. I can revert the latest commit if you don't think it needs it.

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 28, 2013

Really cool. The Safari icon was too small for me, but that's probably a retina thing. I'll look it over in more detail tomorrow.

Yes to the title bar. As long as you're improving things for plugins, I don't suppose you want to look at #999? :-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 28, 2013

The Safari icon was too small for me, but that's probably a retina thing. I'll look it over in more detail tomorrow.

Yeah probably. Does the plugins icon display properly? I just use an NSImageView and [myView setImage:[thePlugin icon]] so I'd have thought it takes care of all that. Just the safari icon was wrong? Strange. Do the icons in say the plugins prefs display properly? I'll let you debug :)

Set a breakpoint on QSPluginUpdaterWindowController.m:L154 to see what image is set (check the representations, just po [thisPlugin icon] should be enough).
Or take a look at the image view itself. It's kind of hard to spot in the .xib as it blends in.

I don't suppose you want to look at #999? :-)

I thought you might ask that ;-)
OK, I'll take a look. I want to make more improvements to the download system anyway. It's silly that if you update plugins you then get a message saying "Quicksilver and all it's plugins are up to date!"

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 28, 2013

OK, I've done what is desired, except actually hooking up the help button to open the help panel.
This is because... it's going to be horribly messy to do so. We can either:

  • try and open the plugins pref pane, select the right plugin, then call the method to show help
  • create an entirely new and independent help panel for showing the help when viewing the main prefs

I'm inclined to go for number 2. Thoughts?
I know @tiennou knows how difficult it is to 'select' an entry in any of the prefs (Catalog, triggers etc). ;-)
I've managed to do it for the catalog and triggers prefs, but in this case we're in a completely different bundle so we'll have to do lots of things like NSClassFromString() and make lots of new methods.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 28, 2013

Oh, and P.S. the help icon I made was a 2s attempt until we can get something better. I didn't see the point of me spending ages to make something rubbish :)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 28, 2013

Oh, and I just saw your small icon bug. It's because the image view wasn't set to scale the image correctly.
Fixed now

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 28, 2013

Oh dear, view based tables are 10.7+ only. I guess this'll have to wait until after we drop 10.6 support. So next next release :)

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 28, 2013

I'm inclined to go for number 2.

Yeah, I had started to work on it and came to the same conclusion. 2 sounds much cleaner.

Oh dear, view based tables are 10.7+ only. I guess this'll have to wait until after we drop 10.6 support. So next next release :)

By next release, do you mean the one we're currently working on? Because I'm on board. :-)

I wonder if we should do a B72 from the current master. That will prevent 10.6 users from being stuck with the catalog scanning lock-ups and the slow Spotlight based file name/label business. But then it holds back cool new stuff like this and synonyms for "the" release. ;-)

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 28, 2013

I have a small concern about the format of the updates. The way it list current and new versions followed by a list of changes implies that those are the changes between the two versions displayed. But really, those are only the changes between the two most recent. I would do it more like this:

Terminal Plugin 2.2.2
Changes
* a change
* another change
You have version 2.2.0

Thanks for looking at #999. It looks like the help button moves around a lot. Can we put it on the parent view instead and just hide it for panes marked as "main"? And then you might not need to mess with mainViewDidLoad. Maybe I shouldn't have merged that other thing so quickly. :-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 28, 2013

Mentioned on IRC, but for the record: 10% of users are still 10.6 so it'd be nice to make v1.0 10.6+.
This pull will have to wait til we drop 10.6 support

I'll fork the #999 fixes to another pull request. I'm gonna rewrite history here.
And I'll use method 2

For this pull:

I'll update the changes you suggest, except use the format:
(Low priority)

Terminal Plugin 2.2.2       (Installed: 2.2.0)
...

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 20, 2013

OK I've tweaked the display of the version numbers as I specified in my comment and also fixed the merge conflict. Since 10.6 is dropped this is good to go

Screen Shot 2013-03-20 at 23 40 21

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 20, 2013

Seems to work well, but I get some artifacts at the bottom of each plug-in's section. Looks like you had them in your screenshot, too.

Screen Shot 2013-03-20 at 2 15 23 PM

Opening and closing the "Changes" makes it go away.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 21, 2013

I'm not quite sure what the problem was (perhaps a retina thing, in which case I don't care :P ) but I've tweaked the sizes a little and made a few adjustments. Let me know how things look for you now

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 21, 2013

I've pushed one more update. I realised that one of the main reasons for doing this was so that users can skip plugin updates (e.g. the 'broken Terminal plugin')

If no plugins are ticked you can click the 'skip updates' button.

Also, I've fixed a crash (hopefully the setIdentifier: one) This will have to go when we move to ARC, but it'll flag up as a merge conflict, so we can step through the code of the ARC project and see if ARC does the retain for us :)

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 21, 2013

Not a retina thing. Like I said, it's evident in your screenshot too. Take a look. 😃

I'll try out the new changes. Didn't you already fix the setIdentifier: crash?

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 21, 2013

The skip option works, but I still see the noise under Changes.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 22, 2013

The skip option works, but I still see the noise under Changes.

Alright, in that case you'll need to debug yourself. On IRC right now if
need be

On 21 March 2013 20:59, Rob McBroom notifications@github.com wrote:

The skip option works, but I still see the noise under Changes.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1354#issuecomment-15239917
.

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 22, 2013

Alright, in that case you'll need to debug yourself.

What have I done? Nooooo! Considering that it goes away when you open/close the details, it's probably an unavoidable bug, but I'll take a look.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 22, 2013

Discussed on IRC:

I've tried playing with it, but it seems that with view based tables there's no way to set padding or anything.

The only alternative is to put it in the top left hand corner

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 25, 2013

Looks better. I don't see the noise. I wanted to see how it handled obsoletes. You can't read all the text on the first line. Not sure what the best solution is for that, but users should be able to see it. And under changes, do we really want to show (null)? Could we prevent the Changes from showing up in that case?

Screen Shot 2013-03-25 at 10 03 20 AM

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 25, 2013

OK, thanks for the testing :)
I'll take a look at it in a few days when I get the chance.

On 25 March 2013 21:06, Rob McBroom notifications@github.com wrote:

Looks better. I don't see the noise. I wanted to see how it handled
obsoletes. You can't read all the text on the first line. Not sure what the
best solution is for that, but users should be able to see it. And under
changes, do we really want to show (null)? Could we prevent the Changes
from showing up in that case?

[image: Screen Shot 2013-03-25 at 10 03 20 AM]https://f.cloud.github.com/assets/154676/298162/f5155914-9554-11e2-8daa-b14564cd399a.png


Reply to this email directly or view it on GitHubhttps://github.com//pull/1354#issuecomment-15394879
.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 23, 2013

OK then, finally updated this! Two more things fixed, hopefully they work for you :)

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 23, 2013

Better visually, but it's not actually installing the new plug-in (to replace an obsolete). I get a notification that it was installed, but the old version remains the only copy. Very possible this was a pre-existing bug. I'll try to track it down and let you know.

skurfer added a commit that referenced this issue Apr 23, 2013
@skurfer skurfer merged commit ee0ff40 into quicksilver:master Apr 23, 2013
@skurfer
Copy link
Member

@skurfer skurfer commented Jul 26, 2013

This pull will have to wait til we drop 10.6 support

The deployment target for the new NIB here is 10.6, which throws some warnings. Feel free to change it and push straight to master.

@pjrobertson pjrobertson deleted the pluginUpdater branch Jul 29, 2013
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 29, 2013

Cheers for the heads up. Done

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 29, 2013

Great. Time for a release branch!

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.

2 participants