search

roman / golden-ratio.el

Automatic resizing of Emacs windows to the golden ratio
MIT License
590 stars 38 forks source link

Use a common way to avoid recursion #45

Closed vkazanov closed 9 years ago

vkazanov commented 9 years ago

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 https://github.com/abo-abo/hydra/issues/64 for more details). Actually, the hydra project owner already had three related issues. :-)

syl20bnr commented 9 years ago

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 commented 9 years ago

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 https://github.com/abo-abo/hydra/issues/64#issuecomment-97420257.

syl20bnr commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 https://github.com/roman/golden-ratio.el/pull/45#issuecomment-97423533.

-syl20bnr-

syl20bnr commented 9 years ago

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 https://github.com/roman/golden-ratio.el/pull/45#issuecomment-97423533.

-syl20bnr-

tarsius commented 9 years ago

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 commented 9 years ago

Thanks @vkazanov , merged.

syl20bnr commented 9 years ago

@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 commented 9 years ago

@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:

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

thierryvolpiatto commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 https://github.com/roman/golden-ratio.el/pull/45#issuecomment-97746208.

-syl20bnr-

thierryvolpiatto commented 9 years ago

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 commented 9 years ago

@vkazanov

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

roman commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

sorry I did not mean to be offending at all.

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