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

don't modify identifiers for strings #1569

Merged
merged 1 commit into from Aug 16, 2013
Merged

don't modify identifiers for strings #1569

merged 1 commit into from Aug 16, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Aug 14, 2013

I think the easiest fix is to just revert the changes that assign an identifier to strings, so that's what I've done.

Maybe we can look at a proper fix after the ARC and release branches are both in master.

I tried two different approaches, and neither worked.

First

  1. Change -[QSObject identifier] to just return the identifier
  2. Create a new method, like - (void)establishIdentifier that does all the stuff currently in -[QSObject identifier]
  3. Call establishIdentifier on any new object that gets created.

That last one is the tricky part. I don't think there are too many places it applies to. (Far fewer than the number of places that simply want to read the identifier.) But I'd never feel confident that we got them all. I did it when calling objectsForEntry, when loading children, and when leaving text-entry mode, which seemed to get everything working, but there were frequent crashes when adding to the objectDictionary. Given that I wasn't thrilled with the approach in the first place, I didn't spend a lot of time on it.

Second

I tried dropping identifierForObject from the string handling category and just setting an identifier in objectWithString. The idea there is that new strings would get a proper identifier, but existing identifiers would be left alone.

That still screws up triggers if you look at them in the prefs, though. To show the trigger in the prefs, it eventually asks for dObject and that causes the identifier to be looked up. If an object doesn't exist with that identifier, it treats it as a string and calls objectWithString which rewrites the identifier thanks to what I changed in that method.

Any object that isn't in memory at the time is assumed to be a string and its identifier gets rewritten, which can cause problems down the line.

fixes #1553
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 15, 2013

:'(

But what about all the things reverting it now breaks (i.e. all the things we fixed!)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 15, 2013

...actually - the bug I said I thought I fixed (in #1505) was #1504. We now know that that was fixed by my latest strings pull request (copy the text editor string).

No need for ids for strings!

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 15, 2013

I was about to say “Can you give me an example?”

Because right-arrowing into multi-line strings is still working, and yeah, the big issues were unrelated.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 15, 2013

Yeah, not sure what strings for identifiers does now... ;-)

:'( all my lovely work gone!

Before I merge, do you think you could do this - if you think it's acceptable of course. It's kind string related and would be nice to have in v1.1.0 so @philostein can blog about his idea of searching paragraphs in QS ;-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 15, 2013

Merge ahead if/when you do that

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 15, 2013

Before I merge, do you think you could do this - if you think it's acceptable of course. It's kind string related and would be nice to have in v1.1.0 so @philostein can blog about his idea of searching paragraphs in QS ;-)

We could put it out there in a pre-release and see what people think. Some things to consider from my very brief testing:

  1. When I ⌘⎋ on a web page, the lines aren’t recognized, so I can’t arrow into them anyway.
  2. You can’t see what your typing is matched on, due to the truncation. Probably unavoidable.
  3. I had to type something like 20 characters before the input became unique enough to select one of my 3 test paragraphs. Hitting ↓ might have been a bit more efficient. 😃
  4. If you have Recent Objects enabled, long paragraphs are far more likely to show up unexpectedly, since they match almost everything you type. Obviously, they aren’t usually the top result, but still. This isn’t really a problem with long strings, though. It goes back to something I’ve always argued: Recent Objects should only include those you’ve used in some way. Not those you’ve simply seen in the interface.

So I don’t think there’s much practical benefit to the change, in general, or for the specific use-case we’re talking about. But if you want to try it and see how people react, we can. I just need to commit ’n’ push.

@philostein
Copy link
Contributor

@philostein philostein commented Aug 15, 2013

We could put it out there in a pre-release and see what people think. Some things to consider from my very brief testing:

When I ⌘⎋ on a web page, the lines aren’t recognized, so I can’t arrow into them anyway.

I ⌘A ⌘⎋ The Guardian's home page, and was able to search the text no problem. it seems to work fine on browser-rendered HTML:

http://dl.dropbox.com/u/157506/Search%20a%20webpage's%20text%20in%20Quicksilver%202.mov

(See how I didn't quite select the paragraph properly for comparison at the end of the video.)

Formatted text can't be browsed, but a trigger can fix that:

http://dl.dropbox.com/u/157506/Searching%20paragraphs%20in%20Quicksilver.mov

You can’t see what your typing is matched on, due to the truncation. Probably unavoidable.
I had to type something like 20 characters before the input became unique enough to select one of my 3 test paragraphs. Hitting ↓ might have been a bit more efficient. 😃

Yes, probably more useful for words near the start of paragraphs, but if there's some unusual string near the end like '$50,000', QS will find the paragraph sharpish.

If you have Recent Objects enabled, long paragraphs are far more likely to show up unexpectedly, since they match almost everything you type. Obviously, they aren’t usually the top result, but still. This isn’t really a problem with long strings, though. It goes back to something I’ve always argued: Recent Objects should only include those you’ve used in some way. Not those you’ve simply seen in the interface.

Yes! That'd be great, just the objects that have been used in commands or Quick Looked.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 16, 2013

Yes! That'd be great, just the objects that have been used in commands or Quick Looked.

Or selected via comma trick. Basically, the same criteria that trigger an update to the mnemonics database.

I think the history changes should be a prerequisite for relaxing the restrictions on name, which is a slightly larger project, so I’m not going to include it here. We need to get the next pre-release out!

skurfer added a commit that referenced this issue Aug 16, 2013
don't modify identifiers for strings
@skurfer skurfer merged commit 066f653 into release Aug 16, 2013
@skurfer skurfer deleted the ident branch Aug 16, 2013
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 21, 2013

@skufer - I remember why I added identifiers to strings now. If you try with this build, you can't ← out of a list of strings (after → into paragraphs). Since the parent ID cannot be set because it is nil.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 21, 2013

That’s true. Well, like I said, we can try again later, but it’s not going to be a simple change.

There are a lot of things you can’t ← out of, anyway. 😉

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 21, 2013

Yeah it's a less important thing than breaking triggers etc. of course. But I don't see a solution unfortunately. Unless we directly add the parent object to the child object meta/data dicts.

I'm not sure how fallacy comes into it, but an interesting link nonetheless ;-)

On 21 Awst 2013, at 20:27, Rob McBroom notifications@github.com wrote:

That’s true. Well, like I said, we can try again later, but it’s not going to be a simple change.

There are a lot of things you can’t ← out of, anyway.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 21, 2013

Yeah it's a less important thing than breaking triggers etc. of course. But I don't see a solution unfortunately. Unless we directly add the parent object to the child object meta/data dicts.

QS seems more than willing to just create a string object when it runs across an unknown identifier in most contexts. I wonder if it could do that here. That is, if the parent's ID isn't registered, just create a new string object using the ID and show that. I think that's what triggers based on strings do anyway: recreate the string object every time the trigger is executed or displayed. 😕

I'm not sure how fallacy comes into it

I was basically saying it’s OK for one thing to be broken because other things are broken, too. When of course none of them should be broken.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 21, 2013

QS seems more than willing to just create a string object when it runs across an unknown identifier in most contexts. I wonder if it could do that here.

My original thought waaay back when I made the original pull. I think if you go and look over there I explain why I added the prefix NSStringPboardType-:-.
If you don't, and you copy something like "SomeExistingObjectIdentifier" to your clipboard/type it in QS it'll get muddled between being a string and resolving to the actual object with that identifier.

On 21 Awst 2013, at 23:50, Rob McBroom notifications@github.com wrote:

Yeah it's a less important thing than breaking triggers etc. of course. But I don't see a solution unfortunately. Unless we directly add the parent object to the child object meta/data dicts.

QS seems more than willing to just create a string object when it runs across an unknown identifier in most contexts. I wonder if it could do that here. That is, if the parent's ID isn't registered, just create a new string object using the ID and show that. I think that's what triggers based on strings do anyway: recreate the string object every time the trigger is executed or displayed.

I'm not sure how fallacy comes into it

I was basically saying it’s OK for one thing to be broken because other things are broken, too. When of course none of them should be broken.


Reply to this email directly or view it on GitHub.

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