clarete / hackernews.el

Hacker News client for Emacs
GNU General Public License v3.0
249 stars 26 forks source link

Add hackernews-open-in-other-window #35

Closed nickdrozd closed 6 years ago

nickdrozd commented 6 years ago

Right now, calling `hackernews' opens the buffer in a different window. This change adds a custom variable to specify whether to open the buffer in the current window or another window. The default value matches current behavior.

nickdrozd commented 6 years ago

Actually, https://github.com/clarete/hackernews.el/pull/31 mentions that pop-to-buffer is customizable. Is there a simpler way to make it do what I want it to do?

basil-conto commented 6 years ago

Is there a simpler way to make it do what I want it to do?

I think the simplest would be to customise same-window-regexps:

(add-to-list 'same-window-regexps "\\`\\*hackernews .*\\*\\'")

This has the added benefit of working with the latest Emacs version (26) and at least as far back as 23.

Starting with Emacs 24, a more robust and infinitely flexible/customisable approach is to harness display-buffer-alist:

(add-to-list 'display-buffer-alist
             (cons "\\`\\*hackernews .*\\*\\'"
                   display-buffer--same-window-action))

or

(add-to-list 'display-buffer-alist
         (cons (lambda (buf _alist)
             (with-current-buffer buf
               (derived-mode-p 'hackernews-mode)))
           display-buffer--same-window-action))

etc.

See (emacs) Displaying Buffers and the docstrings of the aforementioned variables for more information.

I'll see if I can update the README to give some useful pointers like this.

basil-conto commented 6 years ago

For future reference, keep in mind that it is seldom advisable to use switch-to-buffer in Elisp code; instead you should prefer pop-to-buffer* and display-buffer* variants. See the following for more information:

basil-conto commented 6 years ago

Thank you for your attention to this!

nickdrozd commented 6 years ago

Wow, thanks for all the info! Adding hackernews to same-window-regexps did the trick. (I used (add-to-list 'same-window-regexps "\*hackernews.*\*"), which is simpler than your suggestion. Am I doing something slightly wrong there too?)

By the way, that series of refactor commits is an instructive model of Emacs hacking. Great job!

basil-conto commented 6 years ago

I used (add-to-list 'same-window-regexps "\*hackernews.*\*"), which is simpler than your suggestion. Am I doing something slightly wrong there too?)

Yes, because the given regexp is slightly syntactically incorrect and may theoretically result in unexpected behaviour. Backlashes are special to both strings and regexps in Elisp, so they have to be doubly escaped in regexps. You can read more about this in (emacs) Regexps, (elisp) Syntax of Regexps and the last parts of (elisp) Regexp Special, but here's a quick illustration:

(string-match-p "\*hackernews.*\*" "hackernews")       ; ⇒ nil
(string-match-p "\*hackernews.*\*" "*hackernews")      ; ⇒ 0
(string-match-p "\*hackernews.*\*" "*hackernews*")     ; ⇒ 0
(string-match-p "\*hackernews.*\*" "*hackernewspaper") ; ⇒ 0

In other words, the first asterisk is being interpreted literally and the second as a Kleene operator. The correct syntax would be:

(string-match-p "\\*hackernews.*\\*" "hackernews")       ; ⇒ nil
(string-match-p "\\*hackernews.*\\*" "*hackernews")      ; ⇒ nil
(string-match-p "\\*hackernews.*\\*" "*hackernews*")     ; ⇒ 0
(string-match-p "\\*hackernews.*\\*" "*hackernewspaper") ; ⇒ nil

See also the obligatory xkcd reference 1638: Backslashes.

Some people prefer to use the rx macro or its accompanying function rx-to-string instead of writing their own regexps, which is always an error-prone process:

(rx "*hackernews" (* nonl) ?*) ; ⇒ "\\*hackernews.*\\*"

The other difference between our regexps comes down to taste and/or use-case. Regexps can match any part of a string unless they are anchored somewhere:

(string-match-p "\\*hackernews.*\\*" "*hackernews*") ; ⇒ 0
(string-match-p "\\*hackernews.*\\*"
                "and now for *hackernews* something completely different")
  ;; ⇒ 12

It is extremely unlikely that this will ever happen to you; in fact many built-in regexps are written exactly the same as yours. I just prefer to be explicit in my own code:

(string-match-p "\\`\\*hackernews.*\\*\\'" "*hackernews*") ; ⇒ 0
(string-match-p "\\`\\*hackernews.*\\*\\'"
                "and now for *hackernews* something completely different")
  ;; ⇒ nil

The anchors added to the start and end of the regexp make it match only if the rest of the regexp enclosed by them matches the entire given string.

This could also be written using rx:

(rx bos "*hackernews" (* nonl) ?* eos) ; ⇒ "\\`\\*hackernews.*\\*\\'"

HTH.

nickdrozd commented 6 years ago

Again, wow, that is very informative! That should really be on Stack Overflow or somewhere like that so people can read it, not on a rejected pull request for a niche Emacs package :smile:

basil-conto commented 6 years ago

Glad I was able to clear things up. :)

That should really be on Stack Overflow

I didn't think to check before replying, maybe it already is. :) There's also https://emacs.stackexchange.com/ if you haven't come across it.

basil-conto commented 3 years ago

Note that same-window-regexps is considered obsolete, so it's better to use display-buffer-alist; see the updated example in commit 46d41c55485b7ab5a759182987d61b310da1490b, as well as (info "(elisp) Choosing Window Options").