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 set identifiers on pasteboard objects #1730

Merged
merged 1 commit into from Jan 14, 2014
Merged

don't set identifiers on pasteboard objects #1730

merged 1 commit into from Jan 14, 2014

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Dec 19, 2013

fixes #1597

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 22, 2013

So I've tracked that code down to my pull req #1406
I can't find any reason for why I did it (other than thinking "things work better when everything has an identifier" - which isn't the case for strings or by the looks of it pasteboard objects... even if in reality things should really have IDs) so I guess this is fine.
...any ideas Rob?

(P.S. @HenningJ we're getting a merge build failed here :( )

@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 23, 2013

Yeah, I’m not aware of a specific problem that was eliminated by adding the identifier. I assumed it was just done as a “best practice” and that you would correct me if there was something it actually fixed.

Note that I did something similar in #1648: If it’s in the catalog, it gets an identifier. This is less of an issue, since the identifier stored with the trigger can be used to get the correct object from the catalog, but if you disable the relevant entry, you get a similar broken trigger. I just tested by adding lines from a text file to the catalog and making a trigger with one of the lines, then removing the catalog entry.

If you think that scenario is common enough to be considered a problem, we could just use the generated identifier as a key in the object dictionary, but not actually assign it to the object. I’m not sure what the implications of that are, though.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Dec 26, 2013

ok to test

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Dec 26, 2013

don't know what went wrong there. seems to work now

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 14, 2014

OK, for now we'll let this live and see how it plays...the only other reason I could think of why having IDs for these objects if for → into objects (and maintaining a reference to the parent objects - as was the case for strings)
That seems irrelevant here. (aside: right arrowing into 'Clipboard History', you cannot arrow out.)

pjrobertson added a commit that referenced this issue Jan 14, 2014
don't set identifiers on pasteboard objects
@pjrobertson pjrobertson merged commit 06c7fa8 into master Jan 14, 2014
1 check passed
@pjrobertson pjrobertson deleted the i1597 branch Jan 14, 2014
skurfer added a commit that referenced this issue Jan 14, 2014
@skurfer
Copy link
Member Author

@skurfer skurfer commented Jan 14, 2014

right arrowing into 'Clipboard History', you cannot arrow out

Heh. Well, I’m afraid you could fill a book with objects where ← doesn’t take you back to the parent. We should probably look at those. I can start with synonyms and remote hosts.

skurfer added a commit that referenced this issue Jan 17, 2014
skurfer added a commit that referenced this issue Jan 17, 2014
skurfer added a commit that referenced this issue Jan 21, 2014
skurfer added a commit that referenced this issue Jan 25, 2014
skurfer added a commit that referenced this issue Feb 2, 2014
skurfer added a commit that referenced this issue Feb 2, 2014
skurfer added a commit that referenced this issue Feb 5, 2014
skurfer added a commit that referenced this issue Feb 11, 2014
skurfer added a commit that referenced this issue Mar 19, 2014
skurfer added a commit that referenced this issue Apr 14, 2014
skurfer added a commit that referenced this issue May 13, 2014
skurfer added a commit that referenced this issue May 30, 2014
skurfer added a commit that referenced this issue Aug 7, 2014
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