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

Feature level #479

Merged
merged 7 commits into from Oct 30, 2011
Merged

Feature level #479

merged 7 commits into from Oct 30, 2011

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Sep 9, 2011

This implements everything discussed in #341

  • The “Enable advanced features” item has been removed from the Command preferences
  • Proxy Objects are enabled by default
  • “Pull selection from front application instead of Finder” is enabled by default
  • All code that was conditional based on feature level has been made unconditional

That last one demands some elaboration. As a general rule, I searched the entire codebase for these strings and removed references to them.

  • kItemFeatureLevel
  • kFeatureLevel
  • kCuttingEdgeFeatures
  • betaLevel
  • alphaLevel
  • devLevel
  • isRestricted
  • fDEV
  • fALPHA
  • fBETA
  • fSPECIAL
  • meetsFeature

In some cases, this meant removing the if clause, making the code run all the time. In a few cases, it meant changing the condition to an #ifdef DEBUG instead. Please look them over.

Most of this stuff is well tested at this point, as it would have been active for any user with advanced features enabled. The notable exceptions are enableEntry and disableEntry in QSFileSystemObjectSource.m. The (now removed) first line of these methods effectively prevented them from ever being called. Now that they’re active, they still have a limited scope. As you can see, they only affect catalog entries that have “watchTarget” or “watchPaths” set. There are a couple of presets that have this (like ~, /Applications/, etc.) but that’s it. From some limited testing, the QSVoyeur stuff seems to work. Objects are immediately added and removed from the catalog as the filesystem is changed (which is pretty cool), so I’m going ahead with this pull request.

One final thing: The changes to the XIBs in 91224bcc were done with a text editor. I’m mostly lost in Interface Builder and could not find the references to alphaLevel anywhere. I haven’t experienced any problems with the change.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 10, 2011

sounds good! plus it sound like you need someone to comment on it :(

I've always been if favour of removing the feature level, so this is
definitely a step in the right direction. When I'm home (only 2 weeks now)
I'll look over all of this.
Hopefully there won't be a need for me to do so as it will have already been
checked and merged LD

On 9 September 2011 20:00, Rob McBroom <
reply@reply.github.com>wrote:

This implements everything discussed in #341

  • The Enable advanced features item has been removed from the Command
    preferences
  • Proxy Objects are enabled by default
  • Pull selection from front application instead of Finder is enabled by
    default
  • All code that was conditional based on feature level has been made
    unconditional

That last one demands some elaboration. As a general rule, I searched the
entire codebase for these strings and removed references to them.

  • kItemFeatureLevel
  • kFeatureLevel
  • kCuttingEdgeFeatures
  • betaLevel
  • alphaLevel
  • devLevel
  • isRestricted
  • fDEV
  • fALPHA
  • fBETA
  • fSPECIAL
  • meetsFeature

In some cases, this meant removing the if clause, making the code run all
the time. In a few cases, it meant changing the condition to an #ifdef DEBUG instead. Please look them over.

Most of this stuff is well tested at this point, as it would have been
active for any user with advanced features enabled. The notable exceptions
are enableEntry and disableEntry in QSFileSystemObjectSource.m. The
(now removed) first line of these methods effectively prevented them from
ever being called. Now that theyre active, they still have a limited scope.
As you can see, they only affect catalog entries that have watchTarget or
watchPaths set. There are a couple of presets that have this (like ~,
/Applications/, etc.) but thats it. From some limited testing, the
QSVoyeur stuff seems to work. Objects are immediately added and removed from
the catalog as the filesystem is changed (which is pretty cool), so Im
going ahead with this pull request.

One final thing: The changes to the XIBs in 91224bcc were done with a text
editor. Im mostly lost in Interface Builder and could not find the
references to alphaLevel anywhere. I havent experienced any problems with
the change.

You can merge this Pull Request by running:

git pull https://github.com/skurfer/Quicksilver featureLevel

Or you can view, comment on it, or merge it online at:

#479

-- Commit Summary --

  • remove featureLevel and related variables/methods - closes #341
  • enable proxy objects and pull selection from the front application by
    default
  • one more feature level reference removed
  • remove conditional naming of plug-ins based on feature level
  • pref panes load regardless of feature level
  • remove commented feature level code

-- File Changes --

M Quicksilver/Code-App/QSApp.h (3)
M Quicksilver/Code-App/QSApp.m (11)
M Quicksilver/Code-App/QSController.m (16)
M Quicksilver/Code-App/QSHelpersPrefPane.m (1)
M Quicksilver/Code-App/QSMainPreferencePanes.h (2)
M Quicksilver/Code-App/QSMainPreferencePanes.m (24)
M Quicksilver/Code-App/QSPlugInsPrefPane.m (2)
M Quicksilver/Code-App/QSPreferencesController.m (2)
M Quicksilver/Code-QuickStepCore/QSAction.m (6)
M Quicksilver/Code-QuickStepCore/QSCatalogEntry.h (1)
M Quicksilver/Code-QuickStepCore/QSCatalogEntry.m (28)
M Quicksilver/Code-QuickStepCore/QSCore.h (1)
M Quicksilver/Code-QuickStepCore/QSExecutor.m (10)
D Quicksilver/Code-QuickStepCore/QSFeatureLevel.h (13)
M Quicksilver/Code-QuickStepCore/QSKeys.h (1)
M Quicksilver/Code-QuickStepCore/QSPlugIn.h (1)
M Quicksilver/Code-QuickStepCore/QSPlugIn.m (23)
M Quicksilver/Code-QuickStepCore/QSPreferenceKeys.h (2)
M Quicksilver/Code-QuickStepFoundation/NSApplication_BLTRExtensions.h (1)
M Quicksilver/Code-QuickStepFoundation/NSApplication_BLTRExtensions.m (7)
M Quicksilver/Code-QuickStepInterface/QSFileConflictPanel.m (3)
M Quicksilver/Code-QuickStepInterface/QSInterfaceController.m (6)
M Quicksilver/Code-QuickStepInterface/QSObjectView.m (4)
M Quicksilver/Code-QuickStepInterface/QSPreferencePane.m (3)
M Quicksilver/Code-QuickStepInterface/QSResultController.m (3)
M Quicksilver/Code-QuickStepInterface/QSSearchObjectView.m (17)
M Quicksilver/Nibs/QSAdvancedPrefPane.xib (32)
M Quicksilver/Nibs/QSApplicationPrefPane.xib (1436)
M Quicksilver/Nibs/QSSearchPrefPane.xib (1336)
M
Quicksilver/PlugIns-Main/QSCorePlugIn/Code/QSActionProvider_EmbeddedProviders.m
(4)
M Quicksilver/PlugIns-Main/QSCorePlugIn/Code/QSDefaultsObjectSource.m (17)
M Quicksilver/PlugIns-Main/QSCorePlugIn/Code/QSFileSystemObjectSource.m (4)
M Quicksilver/PlugIns-Main/QSCorePlugIn/QSCorePlugIn-Info.plist (2)
M Quicksilver/Quicksilver.xcodeproj/project.pbxproj (4)
M Quicksilver/Resources/QSDefaults.plist (2)

-- Patch Links --

https://github.com/quicksilver/Quicksilver/pull/479.patch
https://github.com/quicksilver/Quicksilver/pull/479.diff

Reply to this email directly or view it on GitHub:
#479

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 11, 2011

Hopefully there won't be a need for me to do so as it will have already been checked and merged

Don’t hold your breath. I've heard barely a peep out of anyone on any subject. I’m sure it’s only temporary though.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 26, 2011

Well, it's still hanging about... so here I am! :)

I've properly read over the pull req message - the QSVoyeur stuff sounds awesome if it works. It's been a feature request for a while.

I'm going to concentrate on Etienne's update-system stuff first and try and get that out the door, then this ;)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 26, 2011

I'm going to concentrate on Etienne's update-system stuff first and try and get that out the door, then this ;)

Sounds good.

@sadhuroura
Copy link

@sadhuroura sadhuroura commented Oct 5, 2011

could anybody help me, I'm trying to try the pull requests but I'm unable to do it. I'm not familiar with it. I've already created a branch called featurelevel. I know I must pull but which direction I ought to use? The current URL: #479 ???

Thank you!

:)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 5, 2011

Click the little i in the green bar below. It’ll give more detailed instructions. (Ignore step 3.)

@sadhuroura
Copy link

@sadhuroura sadhuroura commented Oct 5, 2011

green bar below? sorry

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Oct 6, 2011

I think you can only see the green bar if you have push access to the repository. Here are the instructions:

Step 1: Check out a new branch to test the changes — run this from your project directory
git checkout -b skurfer-featureLevel master
Step 2: Bring in skurfer's changes and test
git pull https://github.com/skurfer/Quicksilver.git featureLevel

Since you already created a featurelevel branch, you might want to change the first step to check out the branch instead of creating a new one.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 24, 2011

So as you can see, this now has a merge conflict. Anyone know if I can clean this branch up so it’ll go away?

I tried rebasing the branch from master, fixing the conflict, redoing the small change to the XIB, and pushing, but it doesn’t like that.

To git@github.com:skurfer/Quicksilver.git
 ! [rejected]        featureLevel -> featureLevel (non-fast-forward)
error: failed to push some refs to 'git@github.com:skurfer/Quicksilver.git'
To prevent you from losing history, non-fast-forward updates were rejected
Merge the remote changes (e.g. 'git pull') before pushing again.  See the
'Note about fast-forwards' section of 'git push --help' for details.

Should I cancel this request, delete the branch (on GitHub) and then push my modified local copy and make a new pull request? Or can this one just be updated somehow?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 24, 2011

Non fast-forward error means you're trying to remove a more recent commit
from your branch. You can get past this error by using the git push -f flag
(f for force)

If it's easier with you, I'm happy for you to go ahead and issue a new pull
request, referencing the old one.

P.S. bloody quick with the triggers merge! :o
P.P.S. I'm working on crash protection as we speak. I think I have it sorted
now :)

On 24 October 2011 20:52, Rob McBroom <
reply@reply.github.com>wrote:

So as you can see, this now has a merge conflict. Anyone know if I can
clean this branch up so itll go away?

I tried rebasing the branch from master, fixing the conflict, redoing the
small change to the XIB, and pushing, but it doesnt like that.

To git@github.com:skurfer/Quicksilver.git
! [rejected] featureLevel -> featureLevel (non-fast-forward)
error: failed to push some refs to 'git@github.com:
skurfer/Quicksilver.git'
To prevent you from losing history, non-fast-forward updates were
rejected
Merge the remote changes (e.g. 'git pull') before pushing again. See
the
'Note about fast-forwards' section of 'git push --help' for details.

Should I cancel this request, delete the branch (on GitHub) and then push
my modified local copy and make a new pull request? Or can this one just be
updated somehow?

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

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 24, 2011

P.P.S. I'm working on crash protection as we speak. I think I have it sorted now :)

I hope you aren’t adding to the (now merged) failedPluginLoad branch. :)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 24, 2011

OK, git push -f seems to have worked and the proper commits are still showing above.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 24, 2011

I hope you aren’t adding to the (now merged) failedPluginLoad branch. :)

Uuuuum... maybe :o

I've incorporated the failedPluginLoad branch into my new fixes. Basically QS now has a single .plist pref for if it crashed or not. It then determines if it was because of a plugin or some other reason.

Hopefully I'll be able to re-push to the closed pull request?! :/

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 24, 2011

My guess is that you can open a new pull request from the same branch and it’ll just include commits that aren’t already in master. Sorry to be so quick. :)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 24, 2011

Sorry to be so quick. :)

Don't be sorry - it's awesome! :D

On 24 October 2011 21:51, Rob McBroom <
reply@reply.github.com>wrote:

My guess is that you can open a new pull request from the same branch and
itll just include commits that arent already in master. Sorry to be so
quick. :)

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

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 24, 2011

I haven't started looking over the code properly yet, but what is the case with plugins here?

Previously, the list of plugins would vary depending on the app level e.g. 'pre' would show all the beta plugins.
Now, will we just be showing everything to everyone?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 25, 2011

Now, will we just be showing everything to everyone?

Feature level no longer does anything, so yeah.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 25, 2011

OK. In that case the plugins repo needs modifying to ignore the PRE/DEV level (that will no longer be given by QS) and to remove all the details of this in the database etc.

@tiennou - could you help with that?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 25, 2011

I was wondering if we should put an #ifdef DEBUG in QSPaths.h so we could point to a different set of paths when testing the server-side. And now that you bring up the above, we could also use the alternate location to put up plug-ins that aren’t ready for release. (Although, I’m not sure we really need an easy in-app distribution method for these. If you’re testing such a plug-in, you’ll probably be building it from source anyway.)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 25, 2011

If youre testing such a plug-in, youll probably be building it from
source anyway

Yeah, I'd say we wouldn't need to bother with this.

It may be worth coming up with another path now for testing purposes :)

On 25 October 2011 18:55, Rob McBroom <
reply@reply.github.com>wrote:

I was wondering if we should put an #ifdef DEBUG in QSPaths.h so we
could point to a different set of paths when testing the server-side. And
now that you bring up the above, we could also use the alternate location to
put up plug-ins that arent ready for release. (Although, Im not sure we
really need an easy in-app distribution method for these. If youre testing
such a plug-in, youll probably be building it from source anyway.)

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

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 26, 2011

I have set up a test plugins system which is just an exact copy of the current system at:

http://qs0.qsapp.com/test (as opposed to the now http://qs0.qsapp.com/plugins )

I'm not sure how much it will break (I think I got everything right...) so it's probably worth testing.
I'm off to bed :)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 27, 2011

Dammit we really should have looked at this before doing all the plugins system stuff. Then we won't have needed to code all the PRE/DEV stuff into it.

I've made changes to qs0/test/ so it should work with this build now.

A few comments:

  • I still see release type stuff in the 'extras' preference pane

...OK just done a search for @"PRE" and I see it still exists.

Let me get this straight: have your changes JUST changed things to do with "advanced features" and have nothing to do with the 'final release' 'pre release' and 'developer release' settings?
If so - that's great! We don't need to change anything :D

...OK now this is REALLY confusing me... are feature level and application update level completely different things?

I've had a quick look through the code and it seems like this affects:

a) plugin & action loading (plugins can set the level for actions)
b) Proxies
c) a bit of catalog stuff you've mentioned

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 27, 2011

By 'I edited the .xibs by hand in a text editor' I'm wondering: how the hell did you know what to remove?!

Comparing the extras pref pane for ß61 and this branch, I see no differences. What exactly did you change in 91224bc ?

Seems good to me atm
I'm getting all the plugins showing which is correct, and it turns out the QS filters the plugins itself as opposed to stuff happening server side, so nothing needs to be changed with the plugins system (esp. as application update type is sticking around by the looks of things)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 27, 2011

P.S. as for the QSVoyer stuff, we need to start using that ;-)
Any ideas what the overheads would be if we added an option: rescan catalog: when files are changed, every 10 minutes, every 30 minutes... So QS used QSVoyer on every folder?

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Oct 27, 2011

P.S. as for the QSVoyer stuff, we need to start using that ;-)
Any ideas what the overheads would be if we added an option: rescan
catalog: when files are changed, every 10 minutes, every 30 minutes...
So QS used QSVoyer on every folder?

I didn't even know that existed. But if it works and isn't too much overhead, I think there shouldn't be any rescan interval for folders/files. Instead "when files are changed" should always be used.

But this kind of seems off-topic for this pull request. :-P

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 28, 2011

By 'I edited the .xibs by hand in a text editor' I'm wondering: how the hell did you know what to remove?!

I searched the files for “alphaLevel” and made an educated guess. :)

Comparing the extras pref pane for ß61 and this branch, I see no differences. What exactly did you change in 91224bc ?

The alphaLevel thing was preventing those pref panes from loading at all. That’s the only difference.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 28, 2011

I searched the files for alphaLevel and made an educated guess. :)

Seems like things work fine in the app, but I'm a bit worried about that!
I'll edit the .xibs in IB and compare the two.

The alphaLevel thing was preventing those pref panes from loading at all.
Thats the only difference.

OK, makes sense :)

Just finally to get this strait: your changes don't affect what type of
'release' of QS can be downloaded (pre, dev, final) but it does affect which
plugins are displayed and which actions can be loaded, as well as proxies
and a few other things.

Thanks

On 28 October 2011 14:56, Rob McBroom <
reply@reply.github.com>wrote:

By 'I edited the .xibs by hand in a text editor' I'm wondering: how the
hell did you know what to remove?!

I searched the files for alphaLevel and made an educated guess. :)

Comparing the extras pref pane for 61 and this branch, I see no
differences. What exactly did you change in 91224bc ?

The alphaLevel thing was preventing those pref panes from loading at all.
Thats the only difference.

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

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 28, 2011

Just finally to get this strait: your changes don't affect what type of
'release' of QS can be downloaded (pre, dev, final) but it does affect which
plugins are displayed and which actions can be loaded, as well as proxies
and a few other things.

Yeah, didn't have tim to answer that earlier, sorry.

I didn't catch that, or I might have removed it too. :) Sounds like we want to keep it, so that's good. From what I can see, my changes don't affect the behavior that preference controls, so we should be good.

For what it's worth, I've been running with these changes merged in since I submitted the pull request and haven't had any problems.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 28, 2011

Cool, sounds good. But you DID edit QSApplicationPrefPane.xib using IB? That was just a matter of deleting and moving stuff up?

Just tried looking at the other two and... I don't have any ideas where alphaLevel would have got in to that so... yeah!

Looks good to me. I'll try and break those pref panes now somehow...
Have you been running with it fine even after the switch to the new plugins system? As I've previously said, it shouldn't affect that at all, but just incase :)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 28, 2011

Cool, sounds good. But you DID edit QSApplicationPrefPane.xib using IB? That was just a matter of deleting and moving stuff up?

Correct. (Well, it wasn’t IB ‘cause that’s gone, but you know.)

Just tried looking at the other two and... I don't have any ideas where alphaLevel would have got in to that so... yeah!

Right? It’s in the “prevent this XIB from loading” section, I suppose.

Looks good to me. I'll try and break those pref panes now somehow...
Have you been running with it fine even after the switch to the new plugins system? As I've previously said, it shouldn't affect that at all, but just incase :)

Yes, but not as long. I only merged the two together after the plug-in stuff got pushed to master.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 28, 2011

Installing QS from scratch looks good. I see proxy objects etc. :)

I'll play with it on my normal install for a little to make sure it's OK

On 28 October 2011 18:56, Rob McBroom <
reply@reply.github.com>wrote:

Cool, sounds good. But you DID edit QSApplicationPrefPane.xib using IB?
That was just a matter of deleting and moving stuff up?

Correct. (Well, it wasnt IB cause thats gone, but you know.)

Just tried looking at the other two and... I don't have any ideas where
alphaLevel would have got in to that so... yeah!

Right? Its in the prevent this XIB from loading section, I suppose.

Looks good to me. I'll try and break those pref panes now somehow...
Have you been running with it fine even after the switch to the new
plugins system? As I've previously said, it shouldn't affect that at all,
but just incase :)

Yes, but not as long. I only merged the two together after the plug-in
stuff got pushed to master.

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

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 30, 2011

I couldn't break anything, and I've had a look through the code for the 3rd time - all good :)

The only thing that could be changed is removing the 'feature' key from all the actions etc of the CorePlugin-Info.plist file, but that's no biggy. It'd probably save the world a few 100kb and a few µJ :)

pjrobertson added a commit that referenced this issue Oct 30, 2011
@pjrobertson pjrobertson merged commit 030aa13 into quicksilver:master Oct 30, 2011
@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 31, 2011

The only thing that could be changed is removing the 'feature' key from all the actions etc of the CorePlugin-Info.plist file, but that's no biggy.

For the sake of keeping it clean, that’s probably a good idea. Not exactly at the top of the list, though.

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

4 participants