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

Various fixes #1029

Merged
merged 12 commits into from
Aug 14, 2012
Merged

Various fixes #1029

merged 12 commits into from
Aug 14, 2012

Conversation

pjrobertson
Copy link
Member

See the commits for more info.

If anybody starts complaining about the text entry background colour not being the same as the interface - it's the fault of the interface, and the lack of standards.
Some interfaces use the QSAppearanceColor1B as the background colour, whilst others seem to use the 3T colour, so there's no consistency. If you don't like the colour then you can just change it in the prefs - something you couldn't do before :)

It took me ages to get the radiuses right on the text edit mode, as there's no consistent way across interfaces for setting that, so I hope you're happy ;-)

Most of these changes are things I did during the 64 bit switch. They were stuck in a git stash so I wanted to get them out before I forgot about them!

@skurfer
Copy link
Member

skurfer commented Jul 31, 2012

Good stuff in here by the way. I didn't mean to jump right in with the nitpicking, just something I noticed on the way out the door. :-)

@pjrobertson
Copy link
Member Author

Totally fine, good stuff can always be improved :)

When you have more time to think (during the 64 bit changes), you tend to
pick up on all the small annoying things :)

On 1 August 2012 00:59, Rob McBroom <
reply@reply.github.com

wrote:

Good stuff in here by the way. I didn't mean to jump right in with the
nitpicking, just something I noticed on the way out the door. :-)


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

@skurfer
Copy link
Member

skurfer commented Aug 1, 2012

The selection service thing

Sorry, I don't have a handle on all the ways selections make their way into QS. What's supposed to be different here? Is this to address the problem people have when the Dock icon is enabled and they sue "Current Selection" twice in a row? I've tried selecting text in Terminal, then "Current Selection → Large Type". It works the first time, but not the second. With or without a Dock icon. I think it might be because the window itself loses focus, even if the application is still active. I've also tried ⌘⎋ twice in a row and it fails the second time (I think for the same reason).

I tried Terminal, TextMate, and Safari and the frontmost window always becomes inactive when pulling a selection into Quicksilver, so there might not be anything we can do here.

Folder children

Looks good. Hard to tell with an SSD, but I'll take your word that it's faster. It certainly doesn't look as though it would be slower.

Searching URLs

We've already gone over this. I think it'll be a great help to users. (Maybe we should open an issue for that bug you found. I think there are other characters besides ? that kill matching.)

Comma trick duplicates

Never even noticed this, but it should work the way you've made it here. Good catch.

Should we just make it a set instead of an array to avoid this testing, or would that require too many changes in other places (that expect an array)? Sets don't preserve order either, and I know there are some actions that can only handle one object, so they take the first or last. So… maybe ignore me. :-)

Text mode UI

Love this. I never liked the way the text would appear next to the icon behind the text you were typing. It's especially noticable in Nostromo.

I didn't find any interfaces where the results were bad. The "numeric result" icon returned by the calculate action is a little off-putting with this, though. :-)

Search Optimization

I remember talking about this before the 64-bit era. Again, I can't really tell a difference, but it seems that this could only be an improvement. The only thing I question is the placement of the new line. Well, that and the use of spaces to indent it, when the rest of the project uses tabs. ;-)

There seem to be some checks on newResultArray and validSearch later in the method. Should we wait for the various tests to pass before accepting the results and assigning them to searchArray? Basically, anywhere that calls [self setResultArray:newResultArray] would immediately call [self setSearchArray:newResultArray] too. Which makes me wonder, could the call to set the search array just be moved inside the setResultArray method to ensure that it always happens? Or would there be other undesirable implications to that?

…s then use the service on the previous app"

This reverts commit 4662962.
Doesn't work as advertised
@pjrobertson
Copy link
Member Author

The selection service thing

Hmmm.... I wish I'd sent a pull request when I actually did this. I'm sure I got it working, but alas... I can't now. I'm just reverting.

So... maybe ignore me :-)

OK ;-)

The "numeric result" icon returned by the calculate action is a little off-putting with this, though. :-)

Not sure what you mean there. From the calculator plugin? It's been around for ages!

Search Optimization

OK, another one I wish I'd committed at the time. Seems like QS is already filtering stuff, but not in some cases. I've moved the call a little so it also filters after you've drilled down into something (SearchFilter as opposed to SearchFilterAll)
I don't think there's much point moving the call to setResultArray, it would just lead to more overhead I think (e.g. cases such as when you have snap to best enabled. The results would all change order but the results to search would be the same)

I've added a few more commits

@skurfer
Copy link
Member

skurfer commented Aug 12, 2012

Not sure what you mean there. From the calculator plugin? It's been around for ages!

Yeah, I knew about it. But it didn't appear behind the text you were typing before. That's what I was referring to. :-)

I'll look over the new commits when I can.

@pjrobertson
Copy link
Member Author

Yeah, I knew about it. But it didn't appear behind the text you were
typing before. That's what I was referring to. :-)

Sure it did! You mean the the answer to the calculation shows behind the
text? That's aaaaaaalways been there!

Like this (The '10')
http://d.pr/i/BS0T

Anyways, I guess it doesn't matter eh?

On 12 August 2012 04:17, Rob McBroom notifications@github.com wrote:

Not sure what you mean there. From the calculator plugin? It's been around
for ages!

Yeah, I knew about it. But it didn't appear behind the text you were
typing before. That's what I was referring to. :-)

I'll look over the new commits when I can.


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

@skurfer
Copy link
Member

skurfer commented Aug 13, 2012

Since this is a hodgepodge of fixes, could you fix line 275 of QSSetupAssistant.m? As the warning says, there's no need to call stringWithString: on a literal string.

@pjrobertson
Copy link
Member Author

To much hodgepodging going on nowadays if you ask me.

Fixed, and a few other warnings

@skurfer
Copy link
Member

skurfer commented Aug 14, 2012

I'm good with this, but like I said, should we be squashing commits? I'm afraid more and more things like 6b10f25 (or technically the commit that preceded it) will make git bisect impossible to use in the future.

@pjrobertson
Copy link
Member Author

I think you're probably right. I'll start squashing things from now on.

That commit shouldn't cause too much problems though as you'll just get a
build error, then you'd have to fix the error and continue your git bisect.
But you're right, not perfect

On 14 August 2012 18:49, Rob McBroom notifications@github.com wrote:

I'm good with this, but like I said, should we be squashing commits? I'm
afraid more and more things like 6b10f256b10f25(or technically the commit that preceded it) will make git
bisect impossible to use in the future.


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

@skurfer
Copy link
Member

skurfer commented Aug 14, 2012

That commit shouldn't cause too much problems though as you'll just get a build error, then you'd have to fix the error and continue your git bisect.

Yeah, for small things like this, we can probably figure it out. But keep in mind you'd also have to go back and remove the change before going to the next step in the bisect to avoid conflicts.

@studgeek
Copy link

I like the idea of being able to search the URLs, but at a practical level I find it is kind of "polluting" my global catalog and made it harder to find files, apps, and other things. I think a great feature would be to allow accessing them through a (Catalog) object without them being added to the global catalog - #1066.

@pjrobertson
Copy link
Member Author

Perhaps that's why searching URLs was originally disabled - because it
would pollute the catalog too much. I've never had URLs in my catalog (see
my tip on → into Chrome or Safari to get the bookmarks) so I obviously
haven't seen a difference.

Maybe you could search URLs just when you're in 'browsing' mode. Thoughts
@skurfer on reverting this?
Obviously with your changes at #1062 things will look a bit nicer overall

On 26 August 2012 01:56, David Rees notifications@github.com wrote:

I like the idea of being able to search the URLs, but at a practical level
I find it is kind of "polluting" my global catalog and made it harder to
find files, apps, and other things. I think a great feature would be to
allow accessing them through a (Catalog) object without them being added to
the global catalog - #1066#1066.


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

@skurfer
Copy link
Member

skurfer commented Aug 26, 2012

I'm sure you'll both see my comment on #1066, which I wrote before seeing this here. So that applies here, as well.

As for reverting the change… maybe. There's nothing new in the catalog, so I wouldn't say this is adding pollution, but since URLs can potentially contain so many characters, they are now far more likely to match searches. If you think about it, it's probably not useful to be able to search anything that comes after a ? in the URL (and yet it's currently being matched on). So maybe we could just set the text to the left of ? as the name, but that would screw up anything that relies on the string value instead of getting the URL type explicitly (which is probably a long list).

@studgeek
Copy link

I have found its making hard to find URLs even when I am just searching URLS through Google Chrome -> Other Bookmarks. There are just so many url string matches.

But maybe I have another problem. it also doesn't seem to be prioritizing whole words over letters spread out across the label or url. If you look at this screenshot I am typing "leanstartup" and its picking "How to use Google Contacts as a Unified Address Book" over "The lean startup Arlington Public Library". Its been acting that way for a while, but I tried switching the space behavior and restarting and now its correctly selecting the lean startup entry. Not sure what changed :/.

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