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

Plugin info #697

Merged
merged 27 commits into from
Feb 22, 2012
Merged

Plugin info #697

merged 27 commits into from
Feb 22, 2012

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Feb 14, 2012

It started innocently enough with some tweaks to the stylesheet used for displaying the extended description. “What a stupid thing to be spending time on”, I thought. Then it exploded.

A summary of changes:

  • The stylesheet for documentation has been updated to better support pre, code, and table. The weird treatment of h3 was removed. (The iTunes and Remote Hosts plug-ins are some that showcase the changes well.)
  • Support for the old RTF-based infoFile has been removed. (There were only 2 or 3 plug-ins using this and the info is from 2005.)
  • The “Updated” column now prefers the date of the locally installed plug-in, using the date of the on-line version as a fallback. (It was the other way around, which generated confusion and complaints.)
  • Documentation (extended description) now displays in its own panel, similar to how the old RTF system would have displayed in a separate application. It never really fit comfortably in the info panel.
  • The info panel now shows, you know, info. The old information is still there (rearranged a bit) and new labels have been added to show the version/date for both the locally installed plug-in and the most recent on-line plug-in.
  • The ? button on the info panel now works (and performs the same function as the Plug-In Help menu item).

As with anything visual, I expect people to have different opinions on what works best, so I’m open to suggestions and expect I’ll need to tweak things here and there.

@HenningJ
Copy link
Contributor

Unrelated to this pull request, but I just built QS for the first time on my system. And it started just fine. But it did not work. It took me a while to figure out that it didn't work because I did build the Quicksilver target instead of the Quicksilver Distribution target. Therefore, I didn't have any of the core plugins and nothing worked. Meh. I think there should be at least a warning or something when the core plugins are missing.

But now, on to your pull request...This is really great. Especially the thing about the dates. :-)
Of course I have a couple of notes:

  • I get a lot of messages in the console (at least when running in debug mode) like the following when browsing the plugin list. I think it's trying to find icons for plugins that don't exist locally. But I'm not sure this is actually related to this pull request. Might have been around before. Unable to locate bundle with identifier QSQRCode, using locator { bundle = QSQRCode; }
  • It would be cool to have an "update available" display for installed plugins, when a newer version is available.
  • In the (new) plugins info pane, it should be "Installed Version" and "Latest Version". Not a big difference, but a little clearer.
  • It should also display the categories the plugin belongs to. You know, from the plugin's Info.plist -> QSPlugin -> categories
  • In the plugin's Info.plist is also a description. That could be displayed there as well.
  • I'm not sure if the "Downloadable/Loaded" display makes sense. You can see if a plugin is installed by the checkmark in the plugins list and by the installed version. And if it's in the list, but not installed, it is of course downloadable. The way it is right now is confusing on the first look.

@skurfer
Copy link
Member Author

skurfer commented Feb 15, 2012

It took me a while to figure out that it didn't work because I did build the Quicksilver target instead of the Quicksilver Distribution target.

The instructions in the wiki mention that, but you probably don’t need them so I can see how you missed it. :-)

I think there should be at least a warning or something when the core plugins are missing.

Wouldn’t be a bad idea. I can recognize the problem instantly now I’m so used to it, but it would be nice for newer devs.

I get a lot of messages in the console

I saw that too, but only with DEBUG. It was around before.

It would be cool to have an "update available" display for installed plugins, when a newer version is available.

It would, but I wonder how useful it would be in practice. The date shown for the latest version is fetched by the update system, isn’t it? So if an update was there, a pop-up already would have notified the user. Maybe it would be better to show three states: Update Available/Up to Date/Newer than the latest (as a few of mine are).

In the (new) plugins info pane, it should be "Installed Version" and "Latest Version". Not a big difference, but a little clearer.

Clearer, but there’s not much width to work with. I could move the data below the label, but I want it to be easy to compare the two. I’ll try some different things.

It should also display the categories the plugin belongs to.

Good idea. Same for description. I’ll add them.

I'm not sure if the "Downloadable/Loaded" display makes sense.

I didn’t change it at all. I just moved it to what I thought was a less prominent spot. I suppose it doesn’t tell you much you don’t already know (and it’s not always right for plug-ins you can install without relaunch). But then it isn’t hurting anything, so I don’t know if we should get rid of it.

@HenningJ
Copy link
Contributor

...but it would be nice for newer devs.

Exactly. New devs and confused old ones like me. :-)

I saw that too, but only with DEBUG. It was around before.

Oh, right. Makes sense.

It would, but I wonder how useful it would be in practice. The date shown for the latest version is fetched by the update system, isn’t it? So if an update was there, a pop-up already would have notified the user. Maybe it would be better to show three states: Update Available/Up to Date/Newer than the latest (as a few of mine are).

I think there are two ways to retrieve the plugin list: 1. The update system with the "new stuff available" popup. 2. the little refresh button on the bottom of the l plugin list. I think when you use that, you don't get a popup. Or do you?
And what happens if you get the popup and don't install the updates? It still would be nice to see which plugins aren't up to date.
The three-way display you describe sounds useful for developers. But I wouldn't display anything for "Up to Date". That should be the standard state most plugins will be in most of the time. That would clutter up the display without adding much and would take away attention from the information that might be interesting: the non-standard states ("Update Available" and "Newer than latest")

Clearer, but there’s not much width to work with. I could move the data below the label, but I want it to be easy to compare the two. I’ll try some different things.

You're right, the data should be aligned, so you can compare them easily. Maybe you could put a headline or something above both display, "Versions" or something.

I'm not sure if the "Downloadable/Loaded" display makes sense.

I didn’t change it at all.

I know. I was suggesting you do change it. It doesn't add anything useful and might be a little confusing on first look. So remove it. Simplify. :-)

@skurfer
Copy link
Member Author

skurfer commented Feb 16, 2012

Rather than commit and make you build, etc. when we don’t even know what we want, how about some images? Here are some samples from a reworked info panel. I didn’t remove “Status” yet.

iTunes Info

Firefox Info

I think when you use that, you don't get a popup. Or do you?

I’m pretty sure you don’t.

And what happens if you get the popup and don't install the updates?

And you just happen to be looking through the plug-ins list and opening the info panel during the week between the last pop-up and the next? :-) I thought of that, but it seems like a rare combination of circumstances. Not that I’m ruling it out.

The three-way display you describe sounds useful for developers. But I wouldn't display anything for "Up to Date". That should be the standard state most plugins will be in most of the time. That would clutter up the display without adding much and would take away attention from the information that might be interesting: the non-standard states ("Update Available" and "Newer than latest”)

OK, but now you’re talking about displaying things conditionally. I’m barely keeping my head above water in Interface Builder as it is. :-) Another thing I thought of was changing the appearance of the text, rather than adding more text. Maybe a red tint for out-of-date plug-ins or something. Maybe a tooltip to explain the color as well? But again, I wouldn’t know right off how to accomplish that.

@skurfer
Copy link
Member Author

skurfer commented Feb 16, 2012

Oh, and any ideas on the tiny icons? It looks like the plug-in provides icon (32x32) and smallIcon (16x16). The XIB is bound to icon, but almost all of them come through as 16x16 for me.

@HenningJ
Copy link
Contributor

Here are some samples from a reworked info panel.

Nice.
As I said, I would remove the status. But if you want to leave it in there, at least now it is clear that it is...well...the status.
For the description, I would remove the label/headline-thingy. I think it reads pretty well without it: "ITunes by Alcor, Rob McBroom allows you to...". I guess that's a matter of taste.
I would also put each category on its own line. Again, I don't have really strong feelings either way.

OK, but now you’re talking about displaying things conditionally. I’m barely keeping my head above water in Interface Builder as it is. :-)

Well, actually I think you can't do that in IB itself. But rather in the controller class for the plugins pref pane. Maybe with QSTableView. But I'd have to take a closer look to tell you more.

Oh, and any ideas on the tiny icons?

I was wondering about that, too. I assumed the plugins provided just a small icon.
Does [QSPlugin icon] actually get called when the info panel is displayed?

@skurfer
Copy link
Member Author

skurfer commented Feb 16, 2012

The icon had a setting called Scaling. I changed it from “Proportionally Down” to “Proportionally Up or Down”. Seems to have worked.

As I said, I would remove the status. But if you want to leave it in there, at least now it is clear that it is...well...the status.

I’ll probably leave it. Someone might care and it’s not hurting anything.

For the description, I would remove the label/headline-thingy.

Yeah, I only did that so you’d know why there was so much blank space if the description was null. But since I’ve added a null placeholder, that shouldn’t be an issue.

I would also put each category on its own line.

I would prefer that as well and that’s what I did at first, but how can we get a decent layout not knowing how many we need to display?

@HenningJ
Copy link
Contributor

The icon had a setting called Scaling. I changed it from “Proportionally Down” to “Proportionally Up or Down”. Seems to have worked.

Great. :-)

I would prefer that as well and that’s what I did at first, but how can we get a decent layout not knowing how many we need to display?

Can't you link the elements below to the one above, so they automatically move down when there are more lines?

Yeah, I only did that so you’d know why there was so much blank space if the description was null. But since I’ve added a null placeholder, that shouldn’t be an issue.

Same as above: If there's a description, display it. If not, collapse the text field and move the ones below up.

I'm sure it possible with IB (and even pretty easy) but I also don't know how to do it off the top of my hat. But I can look into it if you want me to.

@pjrobertson
Copy link
Member

Looks good all of this. I'll merge and use it as my main QS for a bit to see what I think. I agree with all of Henning's suggestions about simplifying the window (No 'Up to Date' string and no 'status' part)

I need to play with it first hand to comment though.

@pjrobertson
Copy link
Member

If the 'status' section was removed, then the 'installed' part could be shown conditionally if the plugin is installed or not. I think the N/A is a bit strange. Yeah I know, it means learning more about IB... :P

Do we need the time string for the Latest/Installed version sections?

Can the plugin documentation window close if the sys preferences close?

@pjrobertson pjrobertson mentioned this pull request Feb 18, 2012
@skurfer
Copy link
Member Author

skurfer commented Feb 19, 2012

If the 'status' section was removed, then the 'installed' part could be shown conditionally if the plugin is installed or not. I think the N/A is a bit strange. Yeah I know, it means learning more about IB... :P

What about when you’re clicking from one plug-in to another (or going down the list with arrow keys)? Do you think it would be better if the information was in a consistent location?

Do we need the time string for the Latest/Installed version sections?

Well, I like having it there, but now that you mention it, I can’t say that it absolutely needs to be there. Maybe it could be in a tooltip for the version or something? Maybe the same tooltip could also contain the status (and the status could be modified to let you know there’s a newer version). But really, if we have enough space to just display these things, why make them hard to discover?

Can the plugin documentation window close if the sys preferences close?

That was the original plan, but the way it works by default (hiding if anything other than the prefs has focus) wasn’t too bad, so I left it. But yeah, it should probably be changed.

Speaking of, do you think the ? button is too subtle?

And any idea how we could make ⌘I trigger the info button? Assigning it as a keystroke doesn’t work. I’m guessing the view it’s in isn’t part of the chain that gets the keystroke when a plug-in/trigger/catalog item is selected.

…t exist

the `removeItemAtPath:` method returns YES when the path is nil, masking the failure
* add "Disabled" (installed, but not loaded)
* rename "Uninstalled" to "Not Installed", which is clearer and closer to the truth
@skurfer
Copy link
Member Author

skurfer commented Feb 20, 2012

More commits to address some things discussed here and on #705.

@pjrobertson
Copy link
Member

A few small things:

  • If you tick/untick a plugin, the 'status' doesn't update to reflect this.
  • Some 'status' sections have the file size (15k) whilst others don't
  • What did 91dab1b do?
  • If there's no extended description (e.g. BezelHUD - which has no description, or Clipboard Module - which has no extended description) then the '?' should be greyed out.
  • The QSQRCode module seems to have HTML in the description. Not sure if this should parsed or if the plugin is at fault? Probably shouldn't allow HTML in the description
  • I still see no need whatsoever for the time. It just confuses me that my 'Installed version' is 10 hours later than the latest version. What?! So I don't have the latest version?

@pjrobertson
Copy link
Member

I meant to add to the end of that:

Sorry for being pedantic. All these changes are really, really good and are going to make plugin reading much easier/better. But it's always the case that once you get started, you never know when to stop... as per you saying:

It started innocently enough with some tweaks to the stylesheet used for displaying the extended description.

:)

@skurfer
Copy link
Member Author

skurfer commented Feb 21, 2012

If you tick/untick a plugin, the 'status' doesn't update to reflect this.
Some 'status' sections have the file size (15k) whilst others don’t

Those aren’t new problems, but I guess I can look at them. :-)

What did 91dab1b do?

It refers to the categories you see to the left of the list of plug-ins. (The ones you could edit before #644.)

If there's no extended description (e.g. BezelHUD - which has no description, or Clipboard Module - which has no extended description) then the '?' should be greyed out.

Good idea. The menu item, too. Easy enough.

The QSQRCode module seems to have HTML in the description. Not sure if this should parsed or if the plugin is at fault? Probably shouldn't allow HTML in the description

No documentation has ever suggested that this will work. Not sure how we can enforce it, other than to let developers know when they do it.

I still see no need whatsoever for the time. It just confuses me that my 'Installed version' is 10 hours later than the latest version. What?! So I don't have the latest version?

Oh, the time. For some reason, I thought you wanted to remove the time and date that appear next to the version. You’re right. It’s distracting noise. I don’t even think they use the same time zone (for me anyway).

@pjrobertson
Copy link
Member

It refers to the categories you see to the left of the list of plug-ins. (The ones you could edit before #644.)

How did I miss that! I like what you've done :)

No documentation has ever suggested that this will work. Not sure how we can enforce it, other than to let developers know when they do it.

Yep. We can leave it as is but ask Eric to remove it in a future plugin update.

Oh, the time. For some reason, I thought you wanted to remove the time and date

Sorry, I should have been clearer. Glad you agree though :)

@HenningJ
Copy link
Contributor

Some 'status' sections have the file size (15k) whilst others don't

Why display the file size at all? Is that really useful for anyone? I know, it used to be there before this. But maybe you should remove it.

The QSQRCode module seems to have HTML in the description. Not sure if this should parsed or if the plugin is at fault? Probably shouldn't allow HTML in the description

I agree, HTML shouldn't be allowed there. But for now, you can just strip it out before displaying the description.

Sorry for being pedantic. All these changes are really, really good and are going to make plugin reading much easier/better.

Totally agree. :-)

* return right away if there's an error
* remove the file size for loaded plug-ins
* move things around to make it easier to understand (without altering the result)
…he pane loads

Previously, you would see all plug-ins no matter what the initial selection was.
@skurfer
Copy link
Member Author

skurfer commented Feb 21, 2012

More commits.

  • The file size is gone.
  • The times are gone.
  • The help controls are disabled unless there’s an extendedDescription. (The method that populates the help panel still uses description as a fallback (or “No documentation available”) because it’s possible to invoke the panel for one plug-in, then select another.)
  • The help panel has a different default position.
  • I’ve changed the “Recommended” list to only include plug-ins where recommended is YES or a related bundle is installed on the user’s machine. Previously, it would also include installed plug-ins. I don’t see the point of recommending something that’s already installed. (The list could still include installed plug-ins, but only if they are recommended/related.)
  • When the plug-ins pref pane opened, it was showing all plug-ins (ignoring the category selected on the left). To actually see plug-ins matching the initially selected category, you had to click another category and come back. Now it correctly filters the plug-ins as soon as the pane loads.

If you tick/untick a plugin, the 'status' doesn't update to reflect this.

I looked at this a bit, but I’m not sure what the solution is. The label is bound to the value returned by the status method, but it only checks that when the selection changes. Do I need to tell the containing view to update itself somehow when the checkbox is clicked? There’s a “Continuously Updates Value” setting in the bindings, but that seems like a bad idea.

I agree, HTML shouldn't be allowed there. But for now, you can just strip it out before displaying the description.

I looked into that. If there were a built-in method for NSString that did that (or one in the existing extensions) I’d probably use it, but there isn’t. So it seems like a lot of code to work around (rare) developer mistakes for this one value.

@pjrobertson
Copy link
Member

Yum yum yum. How did users ever use Quicksilver without these changes in place? My god it must have been difficult to use!

If you tick/untick a plugin, the 'status' doesn't update to reflect this.

I think you can solve this with KVO
It's not a biggy though if you're not sure about KVO. I'm not, but I know Etienne's done it in other places in QS so he could definitely do it.

So it seems like a lot of code to work around (rare) developer mistakes for this one value.

Aye. I'll just inform Eric that HTML shouldn't be in the description.

@HenningJ
Copy link
Contributor

Yes. This is really nice. :-)
I have no objections to merging this.

I looked into that. If there were a built-in method for NSString that did that (or one in the existing extensions) I’d probably use it, but there isn’t. So it seems like a lot of code to work around (rare) developer mistakes for this one value.

You're right. I thought as well that there was some easy method to do that. But if there isn't, it's all right to just leave it.

@skurfer
Copy link
Member Author

skurfer commented Feb 21, 2012

It's not a biggy though if you're not sure about KVO.

I’ve done it recently …following an example in a book. :-) I don’t remember it all off the top of my head, but I can look into it.

@skurfer
Copy link
Member Author

skurfer commented Feb 22, 2012

OK, I think this is ready now. Give it a final test.

The KVO business with status was already most of the way there. I just had to set it up as a property (instead of a stand-alone getter) and set it in the places where it should be changed.

@pjrobertson
Copy link
Member

Looks great. One teeny thing. Should the '?' toggle the documentation window? so if I double click it it'll open/close the window? ... :)

@skurfer
Copy link
Member Author

skurfer commented Feb 22, 2012

Should the '?' toggle the documentation window?

Eh, I don’t think so. :-)

@pjrobertson
Copy link
Member

No? OK then!

pjrobertson added a commit that referenced this pull request Feb 22, 2012
@pjrobertson pjrobertson merged commit 3756e9b into quicksilver:master Feb 22, 2012
pjrobertson added a commit that referenced this pull request Feb 22, 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.

3 participants