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

Large Type fixes #1808

Merged
merged 4 commits into from Apr 17, 2014
Merged

Large Type fixes #1808

merged 4 commits into from Apr 17, 2014

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Mar 27, 2014

Addresses the things described in #1804.

skurfer added 2 commits Mar 27, 2014
Prevents some actions from getting confused and appearing when they
shouldn’t
@pjrobertson
Copy link
Member

pjrobertson commented Mar 27, 2014

I was going to say that I agreed with both your propositions on 1804, but I
can do that here :)

The only other thing... Now that you're testing UTI to make sure the file
is plain text, maybe web could also test a file to check if it's rich text
and shoe rich text as well. Cool or what...?

Actually , that reminds me. I made some changes at some point to make
the details string of objects rich text. It means in places like the shelf, clipboard and results list, the details are rich and colourful. Pretty cool, I'll get it out if I can find it :P

No need here, looks good. Will test it though before merging

On Thursday, 27 March 2014, Rob McBroom notifications@github.com wrote:

Addresses the things described in #1804#1804

.

You can merge this Pull Request by running

git pull https://github.com/quicksilver/Quicksilver largeText

Or view, comment on, or merge it at:

#1808
Commit Summary

  • remove other types from file objects
  • only use Large Type with file contents on text files

File Changes

  • M Quicksilver/Code-QuickStepCore/QSObject_StringHandling.mhttps://github.com/Large Type fixes #1808/files#diff-0(4)
  • M Quicksilver/PlugIns-Main/QSCorePlugIn/Code/QSTextSource.mhttps://github.com/Large Type fixes #1808/files#diff-1(2)

Patch Links:

Reply to this email directly or view it on GitHubhttps://github.com//pull/1808
.

@@ -119,6 +119,8 @@ - (void)sniffString {
if (line >= 0) {
[self setObject:[NSDictionary dictionaryWithObjectsAndKeys:[files lastObject] , @"path", [NSNumber numberWithInteger:line] , @"line", nil] forType:@"QSLineReferenceType"];
}
// wipe existing types and set this up as a file
[self setDataDictionary:[[NSMutableDictionary alloc] init]];
Copy link
Member

@pjrobertson pjrobertson Apr 17, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think [self dataDictionary] removeAllObjects] would be the preferred solution right? No need to create a new mutable dict when we can just clear the current one.

@skurfer
Copy link
Member Author

skurfer commented Apr 17, 2014

Made both of the changes you suggested.

@pjrobertson
Copy link
Member

pjrobertson commented Apr 17, 2014

Just as an aside: The clearObjects calls don't @synchronize on data here.

Since this will most likely be in v2.0, as well #1800, then it's nothing to worry about I guess.

pjrobertson added a commit that referenced this pull request Apr 17, 2014
@pjrobertson pjrobertson merged commit 0092f7f into master Apr 17, 2014
@pjrobertson pjrobertson deleted the largeText branch Apr 17, 2014
@skurfer
Copy link
Member Author

skurfer commented Apr 17, 2014

I was thinking everything on master would be in 1.2.0. So should we worry about it (or decide on a direction for #1800) now?

@pjrobertson
Copy link
Member

pjrobertson commented May 1, 2014

I don't think we should put anything more in 1.2.0.
We need to just get it out the door, then focus on v2.0. Every time we put 'new' stuff out in 1.2.0, it means we need to do a pre-release to test it, which just delays everything :(

@skurfer
Copy link
Member Author

skurfer commented May 1, 2014

OK, nothing new. But this is already in and we need to do another pre-release anyway.

@pjrobertson
Copy link
Member

pjrobertson commented May 1, 2014

OK, well let's leave out #1800 for now :)

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

2 participants