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

only store “chosen” objects in history #2092

Merged
merged 53 commits into from Mar 11, 2016

Conversation

Projects
None yet
3 participants
@skurfer
Copy link
Member

skurfer commented Aug 31, 2015

That is, when a user does something to explicitly say “this is the thing I was searching for”, add it to the history and save the mnemonic used. (I’m thinking there’s never a reason to do one without the other in the first pane, so they’re married now. 💒)

I’ve added the two methods to some additional areas that indicate user acknowledgment of a match, like shortCircuit, moveRight, and Quick Look. I added them to insertTab too, but changed my mind. If you did find the thing you want and are tabbing to select a different action, you’re probably about to execute the command, which will take care of things.

I’d consider a hidden pref that makes the history act more like it did before, where just typing random characters adds things to history whether or not you find the result meaningful, but I’m not sure why you’d want that. 😃

This should also fix #2090 by not updating the history when the user is typing.

@1-61803

This comment has been minimized.

Copy link

1-61803 commented Nov 24, 2015

Just a reminder from this post.
I do the following — select an application, right-arrow, select first file by pressing comma, then down-arrow to next file, hit tab and open them. Now select a random object in first pane and open it, next command+[ to browse past objects. Those 2 files are not revealed in the interface as an object, but they do appear 3 times in the Recent Objects catalogue. I use Bezel, but it also happens in Primer. QS 1.2.0, OS X 10.8.5.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 1, 2015

Recent Objects is dead on now, and history is getting there. You can now get back to comma selected “combined objects” and going back further reveals the selection before that, etc. Kinda cool. Or annoying. Debatable.

All that’s left is:

  • Some weirdness with the current selection. If you select an object but don’t do anything with it, then go back through the history, it skips over the most recent thing. That is, if history has [A, B, C] and you select D, going back jumps to B.
  • “Combined objects” appear multiple times in the history. Perhaps as many times as there are objects in the selection? My guess here is that -[QSObject isEqual:] doesn’t correctly handle combined objects.
@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 1, 2015

If I’m reading that right, Travis thinks parentheses don’t match on indexesOfObjectsPassingTest:, but they do. Not sure what the problem is there. I use xctool to build locally and it’s happy.

@pjrobertson

This comment has been minimized.

Copy link
Member

pjrobertson commented Dec 2, 2015

If I’m reading that right, Travis thinks parentheses don’t match on indexesOfObjectsPassingTest:, but they do. Not sure what the problem is there. I use xctool to build locally and it’s happy.

My only suggestion is that Travis is using an older SDK, and _Nonnull is not in that SDK (I've never seen it before, so I'm assuming it's a new thing. Maybe we should set Travis to specify the SDK. Maybe we should merge #2105 😬

Recent Objects is dead on now, and history is getting there

Sorry, I haven't been following this pull. 'Dead' means what in this context? It's been removed?

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 2, 2015

_Nonnull is not in that SDK (I've never seen it before

Me either. That was a gift from Xcode. Maybe I should take it out (and merge your pull request).

'Dead' means what in this context? It's been removed?

Not dead. “Dead on”, as in accurate. 😃

@pjrobertson

This comment has been minimized.

Copy link
Member

pjrobertson commented Dec 3, 2015

Oh wait, I can't read. Apologies!

On 3 December 2015 at 03:50, Rob McBroom notifications@github.com wrote:

'Dead' means what in this context? It's been removed?

Not dead. “Dead on”, as in accurate. [image: 😃]


Reply to this email directly or view it on GitHub
#2092 (comment)
.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 3, 2015

I ran into something else that requires a judgement call on Recent Objects: What do we do with proxies?

Options:

  1. Store the proxy
  2. Store the resolved object
  3. Store nothing

Right now, we do 1 just by default, but I think it should be one of the others. There’s no need to add the proxy itself to the catalog, since it’s already there. I go back and forth.

Arguments for 2: Mostly harmless and might be useful in some cases.

Arguments for 3: If you don’t even know what the resolved object was, are you going to want to (or be able to) search for it?

@1-61803

This comment has been minimized.

Copy link

1-61803 commented Dec 3, 2015

If 2, then a path to a deleted file would turn into a string I guess, a url of a no longer current webpage would come in handy sometimes, etc

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 9, 2015

OK, this should be ready now. I’m really happy with the way history works. Changes since my last comment…

  • The first item in history won’t be skipped when you go back
  • When encountering a proxy object, add the proxy to history and the resolved object to Recent Objects. (Unless it’s a synonym, in which case it will not be resolved before adding to Recent Objects.)
  • Combined objects only appear in the history once

On that last item, I made some additions to -[QSObject isEqual:] so it could understand combined objects. I wanted some tests of the new behavior, so I went ahead and added all kinds of tests for isEqual: (both old and new behavior). 🎉 In the process, I found and fixed two problems with it.

@1-61803

This comment has been minimized.

Copy link

1-61803 commented Dec 9, 2015

Combined objects only appear in the history once

Can you right arrow in combined objects now?

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 9, 2015

Can you right arrow in combined objects now?

Yes, but you always could. What are you expecting right-arrow to do in that situation?

@1-61803

This comment has been minimized.

Copy link

1-61803 commented Dec 9, 2015

Yes, but you always could. What are you expecting right-arrow to do in that situation?

See each object of the combination. In 1.2.0 I can't, but maybe it's my old version.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 9, 2015

See each object of the combination.

We can’t use right-arrow for that. You need it to drill into folders, etc. when selecting multiple items.

I suggested maybe using ⌥⌘A for that. See #1123

@1-61803

This comment has been minimized.

Copy link

1-61803 commented Dec 9, 2015

Hmm…could it have been possible, maybe 6 or 7 years ago? Anyway, ideally a selection, be it with comma-trick or by proxy, would have some methods to dig in, add and remove. Is that feasible code-wise?

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 9, 2015

Is that feasible code-wise?

The code for that is pretty easy (and mostly already there). It’s the UI we need to figure out.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 11, 2015

In the process of adding more tests, I learned some things about combined objects:

  1. Combining objects where at least one of them has no type data yields something pretty useless
  2. While the data dictionary isn’t empty, objectForType: always returns nil

I adjusted isEqual: accordingly. I like it better this way, since we’re just comparing integers first before attempting to loop over more complicated objects.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 14, 2015

The only problem I still see (which I can’t explain since the tests are passing):

  1. Select a few things with the comma trick.
  2. Hit comma on the final object in the selection (even though it would be included anyway)
  3. Run an action on the selection

That makes the final collection with all objects in it appear twice in the history. Annoying, but an edge case I’m not too worried about.

@pjrobertson

This comment has been minimized.

Copy link
Member

pjrobertson commented Dec 14, 2015

The only problem I still see (which I can’t explain since the tests are
passing):

I haven't tested/played with this, but I can say that writing tests for
stuff to do with the UI (comma trick) is complicated. Running tests on just
the views in code doesn't reproduce the same results as actually going
through the motions of hitting keys. That's why I tried to replicate a real
user in my annoying tests
https://github.com/quicksilver/Quicksilver/blob/master/Quicksilver/Quicksilver%20Tests/Quicksilver_Tests.m

On 14 December 2015 at 22:02, Rob McBroom notifications@github.com wrote:

The only problem I still see (which I can’t explain since the tests are
passing):

  1. Select a few things with the comma trick.
  2. Hit comma on the final object in the selection (even though it
    would be included anyway)
  3. Run an action on the selection

That makes the final collection with all objects in it appear twice in the
history. Annoying, but an edge case I’m not too worried about.


Reply to this email directly or view it on GitHub
#2092 (comment)
.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 14, 2015

I’m not really trying to test the UI, so this shouldn’t have those issues. I just looked at how objectValue was being computed in a QSSearchObjectView and simulated that, then did some comparisons. None of it is really specific to QSSearchObjectView. The history stuff is just what called my attention to isEqual:.

That’s not to say there’s no annoying strangeness with the tests, though. You may not have noticed, but I just told Travis to run them again without changing a thing and it passed. I thought it might because I was seeing the same thing locally. The test that fails is on line 225, indicating that combined1 and combined2 are literally the same object. That makes no sense, and if it keeps failing on the first attempt, I might just take it out. It’s more of a pre-test to make sure the next test isn’t passing for the wrong reason.

Anyway, I’m done with this branch if you want to look it over. Just run a few actions, navigate though some folders, etc. Then look at Recent Objects and move around with ⌘[ and ⌘]. What you see there should make sense… finally.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 20, 2015

Anyway, I’m done with this branch if you want to look it over.

Well, that was a lie. I was working on some improvements around the comma trick and it ended up overlapping with this a bit. To avoid conflicts, I’m keeping it all on one branch. There’s good stuff in here.

More History Improvements

  • The keystroke to show all history in the results list (⌃⌘[), which I wasn’t even aware of, behaves more intuitively. Instead of just updating the result array and doing nothing, it now updates the array and shows the results.
  • There’s a new internal command called “Clear Quicksilver History”. I’m sure you can guess what it does.

Comma Trick Improvements

  • Improved the behavior of uncollect:. When an object is removed, update the selection to be something that’s in the collection.
  • When you select things with the comma trick, the behavior of ⌫ makes more sense. It removes the last selected object until there aren’t any more. (I swapped it with ⌥, which removes from the collection, but leaves the current object in the pane. Not sure why you’d want that.)
  • The string value for combined objects is now the string value of each individual object separated by a new-line instead of “Combined Objects”.
  • The little “collection tray” no longer appears if only one thing is selected.
  • The search string is cleared when selecting a new object. This keeps QS from trying to match it against things that probably never matched.
  • When a combined object gets selected in some contexts, it gets displayed as separate items in the collection tray instead of something like “[3 PDF in Downloads]”. This happens when encountering a combined object in the history, using ⌘A to select all, and when the current selection proxy returns multiple items.
  • Added a keystroke (⌃⌘]) to split a combined object back into its components.
  • You can now browse forward and back through items in the collection tray using ⌃[ and ⌃]. Handy for seeing what’s there since the tiny icons might not be enough. You can also delete the selected item.
  • Added documentation for all new methods and many old ones.

The only thing I’m not sure about is whether or not the methods mentioned in DefaultBindings.qskeys need place-holders in QSSearchObjectView. I assumed they did when I added redisplayObjectValue: and explodeCombinedObject, but then I noticed things like collect: and uncollect: only exist in QSCollectingSearchObjectView, so I didn’t add place-holders for the methods to browse the collection.

If we don’t need place-holders, those first two methods can be simplified.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Dec 22, 2015

I found the problem with duplicates in the history and fixed it.

I thought I had fixed the tests, too. They pass every time locally now, but…

((combined1) not equal to (combined2)) failed: ("<40f049f2 857f0000>") is equal to ("<40f049f2 857f0000>”)

How can those objects have the same memory address? It’s a lie!

@skurfer skurfer force-pushed the objectchosen branch from 9bf2f4d to 834c91b Mar 11, 2016

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Mar 11, 2016

Rebased this guy.

@pjrobertson

This comment has been minimized.

Copy link
Member

pjrobertson commented Mar 11, 2016

Cool. Shall we merge and get a pre-release out? Not sure I'm going to get the time to look over your bazillion commits any time soon :(

@pjrobertson

This comment has been minimized.

Copy link
Member

pjrobertson commented on c2c8413 Mar 11, 2016

Why not store the objects in the iSelector?

What if I do this:

"some text" ⇥ append to... ⇥ "File that is deep in some folder"

I'd kinda want that file in my history

This comment has been minimized.

Copy link
Member

skurfer replied Mar 11, 2016

One lame reason and one good one.

  1. That’s how it was before, so I left it.
  2. That would give you a way around the validation. For instance, if you chose “Move to…”, then went back in your history and found a string… 💥

I suppose we could run objects in the history through validation somehow, but that’s a bit more complicated and probably full of edge cases.

This comment has been minimized.

Copy link
Member

pjrobertson replied Mar 11, 2016

2nd reason is OK ;-)
Can that reasoning be added to the comment?

skurfer added some commits Dec 20, 2015

keep the selected object in the collection
redundant, but otherwise the object gets lost when you browse through the collection
test clean-up
* compare memory addresses, since the name is the only other difference and it isn't used by isEqual:
* fix a typo
compare identifiers if either has a value
skip the string comparison only if they're BOTH nil
add returned objects to history
if an action returns a result, this ensures that going back takes you to the previous object
replace \n with \r for compatibility with NDKeyboardLayout
this was broken by the change in line endings on 4705f12
use a different source array to create a different object
trying to avoid false positive matches
when removing from collection, select object to the left
if there is no object to the left, select the last object like before
make ⌃U clear collection too
delete: is implemented pretty far down, while clearObjectValue has specific implementations in higher classes that do the right thing
refactor to handle new collection behavior
* Initiating a new search after redisplayObjectValue will clear the collection
* Move the equality test in setObjectValue to the superclass
* Remove local implementations of setObjectValue/selectObjectValue. They were mostly necessary to clear the collection, which is now handled by redisplayObjectValue.
show the correct search string in history
instead of just wiping it out

@skurfer skurfer force-pushed the objectchosen branch from 58f8f75 to f21dc74 Mar 11, 2016

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Mar 11, 2016

I’ve added some explanation to the commit about why only the first pane has history. (Now, we’re just abusing the ability to rebase.)

I’ve also looked into the visibleString business. See the last commit. I’m glad you said something.

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Mar 11, 2016

I started to add the “explode” action, but ended up scrapping it. Running the action is easy enough, but the validation was too cumbersome/expensive. You would need to get the interface controller, then get the dSelector from that, then check the count on its collection and the collection isn’t even available outside the class, so…

Anyway, I think the rest of this is at least pre-release ready, so expect to see that soon!

skurfer added a commit that referenced this pull request Mar 11, 2016

Merge pull request #2092 from quicksilver/objectchosen
only store “chosen” objects in history

@skurfer skurfer merged commit 7135389 into master Mar 11, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@skurfer skurfer deleted the objectchosen branch Mar 11, 2016

skurfer added a commit that referenced this pull request Mar 11, 2016

@pjrobertson

This comment has been minimized.

Copy link
Member

pjrobertson commented Mar 11, 2016

the validation was too cumbersome/expensive.

Aah ok. But isn't the dObject passed to the validation action anyway? Or are you saying this isn't enough to know the full collection?

The only other big comment I had was about the splitObjects possibly being mutable. I'll look into that in a separate PR though

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Mar 12, 2016

But isn't the dObject passed to the validation action anyway? Or are you saying this isn't enough to know the full collection?

The dObject is available, and I was just using its count, but then I realized that could be greater than 1 for both a collection, and a combined object.

Although, now that I think about it, why shouldn’t you be able to explode either? In fact, it’s probably even more useful when you end up with a single “combined object”. Although, those are going to be pretty rare from now on, since we’re using redisplayObjectValue: all over. (I’ll update the command-line tool after this goes final.)

The only other big comment I had was about the splitObjects possibly being mutable. I'll look into that in a separate PR though

I know I used mutableCopy for that at least once in this branch, but I don’t remember why. To answer your earlier comment, I don’t know of any reason why splitObjects would need to return a mutable array.

@skurfer skurfer referenced this pull request Mar 12, 2016

Merged

Explode Combined Objects #2188

@skurfer

This comment has been minimized.

Copy link
Member

skurfer commented Mar 12, 2016

I know I used mutableCopy for that at least once in this branch, but I don’t remember why.

After adding that action, I think I know why. showArray: expects a mutable array. That’s something else I would question the need for when you go digging. 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment