!URGENT ß74 Fix: Show the icon in the dock if users have never changed the setting #669

pjrobertson · 2012-01-30T08:23:48Z

If 'Show Dock Icon' has never been set in the prefs, there's no corresponding key in the preferences .plist. The old behaviour was to show the icon if there was no key, so we should respect this.

I learnt about this problem here: https://twitter.com/sadhuramrourato/status/163805906470047746

The change just shows the dock icon if the pref is set, or if it doesn't exist.

P.S. sorry for sending this twice. I wanted to try and make it merge automatically into Quicksilver/release france, but alas it doesn't seem possible. I'll get onto GitHub about it.

If 'Show Dock Icon' has never been set in the prefs, there's no corresponding key in the preferences .plist. The old behaviour was to show the icon if there was no key, so we should respect this.

pjrobertson · 2012-01-30T08:24:12Z

I'm more than happy to merge this into ß64 if it's deemed OK. I have the release branch and everything ;)

… it's reflected in QS' prefs

skurfer · 2012-01-30T13:34:14Z

Quicksilver/Code-App/QSApp.m

@@ -59,8 +59,9 @@ - (id)init {

 	// Honor dock hidden preference if new version
 	isUIElement = [self shouldBeUIElement];
-        if (isUIElement && [defaults objectForKey:kHideDockIcon] && ![defaults boolForKey:kHideDockIcon]) {
-		if (![defaults objectForKey:@"QSShowMenuIcon"])
+    if (isUIElement && (![defaults objectForKey:kHideDockIcon] || ![defaults boolForKey:kHideDockIcon])) {


I don’t think you need the objectForKey: test. The documentation for boolForKey: states: “If a boolean value is associated with defaultName in the user defaults, that value is returned. Otherwise, NO is returned.”

Right you are!

It took me a few seconds to get my head round the !NO etc. business, but I
got there!

New commit, let me know what you think.

On 30 January 2012 13:34, Rob McBroom <
reply@reply.github.com

wrote:

@@ -59,8 +59,9 @@ - (id)init {

// Honor dock hidden preference if new version isUIElement = [self shouldBeUIElement];

if (isUIElement && [defaults objectForKey:kHideDockIcon] &&
![defaults boolForKey:kHideDockIcon]) {

if (![defaults objectForKey:@"QSShowMenuIcon"])

if (isUIElement && (![defaults objectForKey:kHideDockIcon] ||
![defaults boolForKey:kHideDockIcon])) {

I dont think you need the objectForKey: test. The documentation for
boolForKey: states: If a boolean value is associated with defaultName in
the user defaults, that value is returned. Otherwise, NO is returned.

Reply to this email directly or view it on GitHub:
https://github.com/quicksilver/Quicksilver/pull/669/files#r396928

pjrobertson · 2012-01-30T14:25:10Z

Hmm... I guess there's no point really in setting the value unconditionally. if the boolForKey method takes account of both cases, as you said, there's no need.

I'll remove it unless you think it's necessary

skurfer · 2012-01-30T14:39:29Z

I don’t see the new commit. Did you push it?

pjrobertson · 2012-01-30T14:56:36Z

Apologies, should be there now.

On 30 January 2012 14:39, Rob McBroom <
reply@reply.github.com

wrote:

I dont see the new commit. Did you push it?

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

skurfer · 2012-01-30T16:00:49Z

OK. Tested in a clean account and the Dock icon shows up.

I actually saw this problem the other day, but didn’t connect the dots. I had an account with Quicksilver in the Dock (permanently) and noticed there was never a glowing . under the icon. Now I know why.

!URGENT ß74 Fix: Show the icon in the dock if users have never changed the setting

skurfer · 2012-01-30T16:04:17Z

I’ve merged it to master. I’ll let you merge it into release.

Are you going to upload a new build to qsapp.com? Unfortunately, I’m just thinking of this now, but should we bump the build number (leaving the version number the same) so pre-release users will get it? If you think so, feel free to make such a commit on your own with no pull request.

pjrobertson · 2012-01-30T16:35:31Z

Are you going to upload a new build to qsapp.com?

I was planning on doing that. I'll go ahead and do it :)
I'll wait another day or so before releasing 65. We said a week, didn't we
:)

Ive merged it to master. Ill let you merge it into release.

I think the easier way of doing it would have just been to merge 'release'
into master once the release has been finalised. Then there's no need to
make commits to two branches.

On 30 January 2012 16:04, Rob McBroom <
reply@reply.github.com

wrote:

Ive merged it to master. Ill let you merge it into release.

Are you going to upload a new build to qsapp.com? Unfortunately, Im just
thinking of this now, but should we bump the build number (leaving the
version number the same) so pre-release users will get it? If you think so,
feel free to make such a commit on your own with no pull request.

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

pjrobertson · 2012-01-30T16:43:55Z

Are you going to upload a new build to qsapp.com?

Oh, I think I see what you're saying now. Well, my thoughts are: this is a
new build, different from 64. It's different from 64, so why give it the
same name?
So this would be something like 65 (3914). Avoid confusion with naming,
numbering and... anyway? What's so bad about incrementing the build number?

On 30 January 2012 16:35, Patrick Robertson robertson.patrick@gmail.comwrote:

Are you going to upload a new build to qsapp.com?

I was planning on doing that. I'll go ahead and do it :)
I'll wait another day or so before releasing 65. We said a week, didn't
we :)

Ive merged it to master. Ill let you merge it into release.

I think the easier way of doing it would have just been to merge 'release'
into master once the release has been finalised. Then there's no need to
make commits to two branches.

On 30 January 2012 16:04, Rob McBroom <
reply@reply.github.com

wrote:

Ive merged it to master. Ill let you merge it into release.

Are you going to upload a new build to qsapp.com? Unfortunately, Im
just thinking of this now, but should we bump the build number (leaving the
version number the same) so pre-release users will get it? If you think so,
feel free to make such a commit on your own with no pull request.

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

skurfer · 2012-01-30T18:47:19Z

I think the easier way of doing it would have just been to merge 'release' into master once the release has been finalised. Then there's no need to make commits to two branches.

But the commits would have to be on the release branch in the first place, which they won't be since GitHub merges pull requests to master. The goals (I thought) were to preserve the GitHub workflow, but also have a known-good branch with only commits that are part of a release.

I don't think we want a free-for-all where sometimes we merge master into release and sometimes we go the other way. Since master will contain everything from various pull requests, we pretty much have to go from that to release, rather than the other way around.

Here's what I had envisioned:

Keep doing what we've always done on master. All new pull requests (when approved) get merged there.
The release branch is only updated in one of two ways:
- Immediately after tagging a commit on master, you do a git checkout release; git merge master to make release look like master at the point of the tag.
- If some problem is found during pre-release but prior to a final release, the fix gets done on master as usual, but the fix branch is also merged into release. That allows release to get the fix without getting all the other stuff that may have been changed on master.
If a problem is found after a final release, we fix it and do a new release (new version, etc.).

In hindsight, it's so easy to create a release branch from an existing tag, I wonder if we even need to keep it around permanently. Seems to me we could just create a release branch as needed in a situation like this.

this is a new build, different from 64. It's different from 64, so why give it the same name?

True, but my last 20 builds said 64 and they were all different. :-) All that really matters is what's released.

So this would be something like 65 (3914). Avoid confusion with naming, numbering and... anyway? What's so bad about incrementing the build number?

I'm all for incrementing the build number so pre-release users will get the new version. I just don't think we should up the version number unless the fix comes after a final release (as mentioned above). So I think current pre-release users should get B64 (3914). We'd chew through version numbers too quickly the other way. (Yes, that's very subjective.)

As for avoiding confusion: I'm assuming the pre-release user base is smaller and more technical, on average. If we keep the version the same (and just increment the build), the few pre-release users that even notice will probably get what's going on. On the other hand, if we do what you suggest, all the regular users (who are still on 63) will eventually jump to 65. Now that will generate confusion and questions. :-)

pjrobertson · 2012-01-30T19:03:23Z

We really need to decide on the best workflow for all of this...!

Probably best to discuss it all in the dev groups, so I'll move it over
there when I get some time (even though we have already discussed it in the
Quick Look branch)

I think the fundamental differences in thinking between us is that:

Your thinking is 6x means a 'final' release. Similar to a X.0.0 (e.g.
1.0.0) for most other apps.
My thinking is that 6x - they're all betas, none of them are 'final'
releases. So I say we increment the version number. It's the equivalent of
going from 1.0.0 to 1.0.1 for me.

Maybe we should just go v1.0 and forget about all this confusion. Then bug
fixes would be 0.0.x etc. :)

On 30 January 2012 18:47, Rob McBroom <
reply@reply.github.com

wrote:

I think the easier way of doing it would have just been to merge
'release' into master once the release has been finalised. Then there's no
need to make commits to two branches.

But the commits would have to be on the release branch in the first place,
which they won't be since GitHub merges pull requests to master. The goals
(I thought) were to preserve the GitHub workflow, but also have a
known-good branch with only commits that are part of a release.

I don't think we want a free-for-all where sometimes we merge master into
release and sometimes we go the other way. Since master will contain
everything from various pull requests, we pretty much have to go from that
to release, rather than the other way around.

Here's what I had envisioned:

Keep doing what we've always done on master. All new pull requests
(when approved) get merged there.

The release branch is only updated in one of two ways:

Immediately after tagging a commit on master, you do a git checkout release; git merge master to make release look like master at the point of
the tag.

If some problem is found during pre-release but prior to a final
release, the fix gets done on master as usual, but the fix branch is also
merged into release. That allows release to get the fix without getting all
the other stuff that may have been changed on master.

If a problem is found after a final release, we fix it and do a new
release (new version, etc.).

In hindsight, it's so easy to create a release branch from an existing
tag, I wonder if we even need to keep it around permanently. Seems to me we
could just create a release branch as needed in a situation like this.

this is a new build, different from 64. It's different from 64, so why
give it the same name?

True, but my last 20 builds said 64 and they were all different. :-) All
that really matters is what's released.

So this would be something like 65 (3914). Avoid confusion with naming,
numbering and... anyway? What's so bad about incrementing the build number?

I'm all for incrementing the build number so pre-release users will get
the new version. I just don't think we should up the version number unless
the fix comes after a final release (as mentioned above). So I think
current pre-release users should get B64 (3914). We'd chew through version
numbers too quickly the other way. (Yes, that's very subjective.)

As for avoiding confusion: I'm assuming the pre-release user base is
smaller and more technical, on average. If we keep the version the same
(and just increment the build), the few pre-release users that even notice
will probably get what's going on. On the other hand, if we do what you
suggest, all the regular users (who are still on 63) will eventually jump
to 65. Now that will generate confusion and questions. :-)

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

HenningJ · 2012-01-31T12:46:03Z

I agree with Rob. With our current version numbers, ß64 is the name for a major release, the build number is the name for a minor bug fix

HenningJ · 2012-01-31T14:18:07Z

BTW:

But the commits would have to be on the release branch in the first place, which they won't be since GitHub merges pull requests to master.

When you make a pull request, you can choose which branch you want to apply the pull request to. Does that not work?

pjrobertson · 2012-01-31T14:25:29Z

I agree with Rob. With our current version numbers, 64 is the name for a
major release, the build number is the name for a minor bug fix

Alright, I'm happy with that. I was going to open a new topic on the dev
groups, but to be honest - it's not worth it. I thought about this whilst
having tea yesterday and thought - whatever's easiest/works for most is the
best bet, so we can all focus our efforts on actual coding :)

Rob - About your mention of not keeping the 'release' branch around
indefinitely. I agree, and think the usual/best way to do things is have a
branch named 'b64 release' or similar, which is then killed when the proper
release is made.
Phew - sounds like we've made it!

On 31 January 2012 12:46, Henning Jungkurth <
reply@reply.github.com

wrote:

I agree with Rob. With our current version numbers, 64 is the name for a
major release, the build number is the name for a minor bug fix

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

pjrobertson · 2012-01-31T14:30:10Z

When you make a pull request, you can choose which branch you want to
apply the pull request to. Does that not work?

Nope, for some reason the 'release' branch wasn't there. That's why I
wanted to get in touch with GH. I'm not sure if it's because the branch got
created after I forked from the QS repo or what.

On 31 January 2012 14:18, Henning Jungkurth <
reply@reply.github.com

wrote:

BTW:

But the commits would have to be on the release branch in the first
place, which they won't be since GitHub merges pull requests to master.

When you make a pull request, you can choose which branch you want to
apply the pull request to. Does that not work?

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

skurfer · 2012-01-31T15:32:26Z

When you make a pull request, you can choose which branch you want to apply the pull request to. Does that not work?

Sure, but if we decided that was the official way from now on, half the people wouldn’t know and the other half would forget. :-) We could have “dev” and “master” and send all pull requests to “dev”, or we could have “master” and “release” and send all pull requests to master. The only real difference is in naming, so I say we do what’s easiest (which is to accept GitHub’s default behavior).

Now, we can talk about maybe doing that for fixes between pre-release and release. That is, open pull requests for fixes on the release branch. But they still need to get applied to master as well somehow. I don’t have strong feelings about how we handle that (but again, not having to remember the exception and just going with GitHub defaults might be best).

HenningJ · 2012-01-31T15:52:16Z

@skurfer: I agree. Go with GitHub defaults, and only do special things when they need to be special.

That comment was meant for Patrick. It seemed that he couldn't get the "pull request to another branch than master" to work, for some reason.

Show the icon in the dock if users have never changed the setting

4ead4ca

If 'Show Dock Icon' has never been set in the prefs, there's no corresponding key in the preferences .plist. The old behaviour was to show the icon if there was no key, so we should respect this.

Tidy up the 'dock icon' check and set the BOOL in the prefs to ensure…

7155523

… it's reflected in QS' prefs

skurfer reviewed Jan 30, 2012
View reviewed changes

Remove unnecessary duplicate test

dbc8ce8

skurfer added a commit that referenced this pull request Jan 30, 2012

Merge pull request #669 from pjrobertson/b64Fix

682b4ac

!URGENT ß74 Fix: Show the icon in the dock if users have never changed the setting

skurfer merged commit 682b4ac into quicksilver:master Jan 30, 2012

!URGENT ß74 Fix: Show the icon in the dock if users have never changed the setting #669

!URGENT ß74 Fix: Show the icon in the dock if users have never changed the setting #669

pjrobertson commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

skurfer Jan 30, 2012

pjrobertson Jan 30, 2012

pjrobertson commented Jan 30, 2012

skurfer commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

skurfer commented Jan 30, 2012

skurfer commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

skurfer commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

HenningJ commented Jan 31, 2012

HenningJ commented Jan 31, 2012

pjrobertson commented Jan 31, 2012

pjrobertson commented Jan 31, 2012

skurfer commented Jan 31, 2012

HenningJ commented Jan 31, 2012

!URGENT ß74 Fix: Show the icon in the dock if users have never changed the setting #669

!URGENT ß74 Fix: Show the icon in the dock if users have never changed the setting #669

Conversation

pjrobertson commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

skurfer Jan 30, 2012

Choose a reason for hiding this comment

pjrobertson Jan 30, 2012

Choose a reason for hiding this comment

pjrobertson commented Jan 30, 2012

skurfer commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

skurfer commented Jan 30, 2012

skurfer commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

skurfer commented Jan 30, 2012

pjrobertson commented Jan 30, 2012

HenningJ commented Jan 31, 2012

HenningJ commented Jan 31, 2012

pjrobertson commented Jan 31, 2012

pjrobertson commented Jan 31, 2012

skurfer commented Jan 31, 2012

HenningJ commented Jan 31, 2012