Use a common way to avoid recursion

[vkazanov]

This patch allows for comfortable temporary disabling of the golden-ratio function, while still avoiding recursion: (let (golden-ratio-mode nil) do-something-with-window-here). Otherwise, it works as intended.

It turned out that hydra's (see https://github.com/abo-abo/hydra) uses a special echo window, and using a usual way to disable a minor mode does not work (see abo-abo/hydra#64 for more details). Actually, the hydra project owner already had three related issues. :-)

[syl20bnr]

If this is merged, don't forget to document it as it is a hack (see the discussion on the referenced issue above and the documentation about minor mode conventions).

Prefere to introduce a new variable inhibit-golden-ratio to be used in a let form.

[tarsius]

I disagree with @syl20bnr. This is not a hack but what you are supposed to do - it does not have to be documented. Adding inhibit-golden-ratio would also work, but that is an unnecessary complication in a case where just consulting the existing mode variable would also work. See abo-abo/hydra#64 (comment).

[syl20bnr]

I fully disagree with this. See my comment on the hydra issue and on the abo-abo PR as well. Using inhibit is both cleaner and simpler implementation wise.

[tarsius]

I think "fully" is a bit of an exaggeration - we now both seem to agree that this package should allow the use of a let-binding to inhibit modified behavior. We only disagree on whether it is necessary to add an additional variable for that and to document this.

[syl20bnr]

There is already an additional intermediate variable in the proposed PR
#46 right ? It can be replaced by an inhibit variable.

Le mercredi 29 avril 2015, Jonas Bernoulli notifications@github.com a
écrit :

I think "fully" is a bit of an exaggeration - we now both seem to agree
that this package should allow the use of a let-binding to inhibit it's
behavior. We only disagree on whether it is necessary to add an
additional variable for that.

—
Reply to this email directly or view it on GitHub
#45 (comment).

-syl20bnr-

[syl20bnr]

This discussion is a bit like using if or when. The semantic provided
is different in both case but pretty fundamental for experienced elisp
coders.

Le mercredi 29 avril 2015, Jonas Bernoulli notifications@github.com a
écrit :

I think "fully" is a bit of an exaggeration - we now both seem to agree
that this package should allow the use of a let-binding to inhibit it's
behavior. We only disagree on whether it is necessary to add an
additional variable for that.

—
Reply to this email directly or view it on GitHub
#45 (comment).

-syl20bnr-

[tarsius]

I really don't care what variable I have to let-bind as long as there is one that is respected by the advice.

... for experienced elisp coders.

Like @abo-abo, I am an experienced elisp coder, and I too am a bit offended your need to throw in such qualifiers.

So using an additional variable might or might not have semantic advantages. What is important though is that this mode provides some way to let-bind it out of effect. But because semantic disagreements were hanging in the air, just nothing happened.

To me it is clear that let-binding a mode variable does not turn of a mode, but just suppresses it's effect temporarily (provided the mode has arranged for that). That's because I have seen it done like this many times before. For other people who have not seen it done that way this might be a bit surprising, so by all means use an inhibit variable and document it, to avoid confusion.

[thierryvolpiatto]

Thanks @vkazanov , merged.

[syl20bnr]

@thierryvolpiatto I don't understand why you merged this PR and not #46 instead. #46 is a better change and only needed the replacement of the variable for an inhibit variable which simplified the implementation and provided a better semantic while being cleaner.

I really need @roman to tell us what he want us to do on this. I did not merged #45 or #46 because I wanted him to raise his voice.

@roman what is your opinion on this ?

[syl20bnr]

@tarsius sorry I did not mean to be offending at all. Did I say such a thing when you tell me to not fork and contribute upstream and suppose that I fork all over the place in Spacemacs ? Nope but I certainly could. But I don't think it bring anything meaningful to say such a thing.

What I meant by experienced elisp coder is that we do care about semantic and I know you are an experienced coder, a lot more than me, so sorry if you missinterpreted but it was just an argument for better semantic, nothing more.

I fully :-) agree that modifying temporarily golden-ratio-mode variable works as intended. But it is not a good implementation!! The inhibit variable is better in every way:

• it better conveys the intent like when vs if
• it simplified the superior PR Simplify `golden-ratio-mode' #46
• it allows to document it in a dedicated docstring of a variable
• the code is more readable because of the confusing (golden-ratio-mode) in a let binding with (golden-ratio-mode) outside (which has dramatically different meaning). With the inhibit variable we have a clear intent of inhibit-golden-ratio

Sorry for the typo, I'm on my phone :-)

[thierryvolpiatto]

Sylvain Benner notifications@github.com writes:

@thierryvolpiatto I don't understand why you merged this PR and not
#46 instead.

#46 is unclean and contain many unrelated and unwanted changes.
#45 is clean and simple and I see no problems with it.

#46 is a better change

It is not, it is an unfinished work.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[abo-abo]

#46 is a better change
It is not, it is an unfinished work.

It may be unfinished, but it's better to advise a single function that's actually related to windows rather than just burn resources in post-command-hook.

Arguably, golden-ratio is still unfinished since it relies on balance-windows. If I had fixed that, there would be no code left of the original: #46 would replace the whole package.

[syl20bnr]

@thierryvolpiatto Can you elaborate on this ?

Le jeudi 30 avril 2015, Thierry Volpiatto notifications@github.com a
écrit :

Sylvain Benner <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> writes:

@thierryvolpiatto I don't understand why you merged this PR and not
#46 instead.

#46 is unclean and contain many unrelated and unwanted changes.
#45 is clean and simple and I see no problems with it.

#46 is a better change

It is not, it is an unfinished work.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

—
Reply to this email directly or view it on GitHub
#45 (comment).

-syl20bnr-

[thierryvolpiatto]

Oleh Krehel notifications@github.com writes:

    #46 is a better change
    It is not, it is an unfinished work.

It may be unfinished, but it's better to advise a single function
that's actually related to windows rather than just burn resources in
post-command-hook.

Probably, I don't remember why I was using post-command-hook so I
believe you.
So no problems, the package is now tagged.
So you can now send a new PR on top of this.
Please make one PR per issue and test it.

Thanks.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[syl20bnr]

@vkazanov

If this is merged, don't forget to document it

[roman]

What is the difference between using nil and -1, and any particular reason this was moved to the let statement and not keep it where it was before?

[thierryvolpiatto]

Roman Gonzalez notifications@github.com writes:

In golden-ratio.el:

@@ -94,15 +95,13 @@ will not cause the window to be resized to the golden ratio."
(loop for fun in golden-ratio-inhibit-functions
thereis (funcall fun))))
(let ((dims (golden-ratio--dimensions))

•      (golden-p (if golden-ratio-mode 1 -1)))
  

•      (golden-ratio-mode nil))
  

What is the difference between using nil and -1, and any particular
reason this was moved to the let statement and not keep it where it
was before?

IIUC they have problems with the hydra package and doing so instead
of disabling the whole mode avoid recursion.
I can't give you more details because I don't use the hydra package.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[roman]

@syl20bnr personally I like this PR better because it is more concise, #46 is a more involved change which requires to be separated into multiple PRs (not suggesting that changes are not a good improvement in readability though) as @thierryvolpiatto states, something like #46 can be merged on top of this PR once it is formalized.

Cheers.

[syl20bnr]

@roman That's fine about #46 but it put some noise on my point which is about bypassing golden-ratio using golden-ratio-mode variable. While doing it this way works and we can find some occurrences of it in Emacs sources, it is not the right way to do it.

Setting the variable golden-ratio-mode to nil tricks Emacs into thinking that the minor mode is disabled, which is not the case because the advices are still running and the state has not been cleaned up. My point is that golden-ratio-mode variable should be manipulated only by its function counterpart golden-ratio-mode to assure that at any given point in time the variable state is in sync with the Emacs state.

So I proposed to create a variable inhibit-golden-ratio which improves the semantic of the user code. It explicitly tells that golden-ratio is still active but inhibited, that is all its side effects are ignored. This is a cleaner approach and this is the one I recommend. It could have been applied to #45 but then there was another PR #46 to superseed #45 so I took this one to explain my point, moreover the inhibit-xxx variable was also simplifying the code of #46, anyway that's not the point :-)

I think semantic is very important and it can greatly improve the code readability, if vs. when is a good example. And in this particular case inhibit not only convey better semantic, but it is also important to keep the state coherent.

Another point about readability, in a let binding I find this very missleading:

(let (golden-ratio-mode) ...)

Because in Emacs (golden-ration-mode) means exactly the opposite than disabling. This is not consistent. I prefer to write and read:

(let ((inhibit-golden-ratio t)) ...)

Moreover as we can find occurrences of this hack in Emacs sources we can also find occurrences of inhibit-xxx variables so I doubt about the validity of this argument.

What do you think about it ?

[syl20bnr]

The example above is not fair because it can be written:

(let ((golden-ratio-mode nil)) ...)

Which is better for readability, but this does not invalidate the points above.

[tarsius]

sorry I did not mean to be offending at all.

@syl20bnr My neither. Sorry if it came across that way.