Update to modern conventions

[andersjohansson]

I really like this package but it’s usage of old conventions made it impossible to deactivate in specific buffers (it specifically interfered with pdf-view buffers).

I made a small effort to update it to modern minor-mode conventions with the definition of a buffer-local and a global minor mode which makes it possible to disable it for specific buffers.

In addition, the bindings are made through the minor-mode keymap, which then unfortunately may get shadowed by other minor modes (so defining overrides for functions defined in other minor-modes may or may not work, this depends on the order of minor-mode-map-alist which depends on loading order). But this still seemed cleaner than switching it on and off in the global keymap.

The second commit removes more of the legacy stuff in the file, but could possibly break people’s configuration as it removes the hooks currently defined. (Instead of whole-line-or-region-on-hook and whole-line-or-region-off-hook, functions that check activation or deactivation by the value of whole-line-or-region-mode could be added to whole-line-or-region-minor-mode-hook. Instead of lwhole-line-or-region-load-hook a (with-eval-after-load) could be used.

I don’t know if many people on MELPA depends on the package and it’s old conventions so that this breakage would be disturbing, but for me the update makes much sense. I haven’t yet tested it extensively either but basically it seems to be working exactly as before.

[andersjohansson]

I also don’t know if you, @purcell, are interested at all in taking these decisions since your maintainership role is stated as "likely temporary".

Cheers anyway!

[purcell]

This is awesome and I'm very much in favour of it. Thanks so much for taking the time to prepare and submit it. May I suggest two further small changes? -

• Please call the global mode whole-line-or-region-global-mode instead. I personally prefer this naming style for global modes, because it retains the consistent package prefix.
• Would you mind updating the license boilerplate to GPLv3, ie. the copyright notice that M-x auto-insert produces when called in an empty .el file in a recent Emacs?

[andersjohansson]

Done! Yes, keeping the prefix is certainly more reasonable. I suppose the four commits could be squashed but perhaps it isn't terribly important right now.

[purcell]

Awesome, thanks!

[purcell]

I've merged this, but then I've gone ahead and changed things a little, such that whole-line-or-region-mode is still the global mode, and there's a local mode called whole-line-or-region-local-mode. This is to avoid disruption to existing configs, but I will investigate smoothing the name migration later, perhaps by deprecating whole-line-or-region-mode for a while.

[jguenther]

I'm getting recursive load errors when trying to (require 'whole-line-or-region-mode) or in autoload of this package (on emacs HEAD).

Looks like during load, custom-declare-variable ends up calling (whole-line-or-region-bind-keys), which seems to try to autoload the package again, and repeats.

(error "Recursive load" "/Users/jguenther/.emacs.d/elpa/whole-line-or-region-20170814.20/whole-line-or-region.el" [... repeated 5x])
  (whole-line-or-region-bind-keys)
  (lambda (symbol newval) (set symbol newval) (whole-line-or-region-bind-keys))(whole-line-or-region-extensions-alist ((copy-region-as-kill whole-line-or-region-copy-region-as-kill nil) (kill-region whole-line-or-region-kill-region nil) (kill-ring-save whole-line-or-region-kill-ring-save nil) (yank whole-line-or-region-yank nil)))
  custom-initialize-reset(whole-line-or-region-extensions-alist '((copy-region-as-kill whole-line-or-region-copy-region-as-kill nil) (kill-region whole-line-or-region-kill-region nil) (kill-ring-save whole-line-or-region-kill-ring-save nil) (yank whole-line-or-region-yank nil)))
  custom-declare-variable(whole-line-or-region-extensions-alist '((...)))
  eval-buffer(#<buffer  *load*-335176> nil "/Users/jguenther/.emacs.d/elpa/whole-line-or-region-20170814.20/whole-line-or-region.el" nil t)  ; Reading at buffer position 11029
  load-with-code-conversion("/Users/jguenther/.emacs.d/elpa/whole-line-or-region-20170814.20/whole-line-or-region.el" "/Users/jguenther/.emacs.d/elpa/whole-line-or-region-20170814.20/whole-line-or-region.el" nil t)
[repeated 5x]

[purcell]

Yup. I've rolled back the master version for now, and will revisit & fix the issue in a few hours.

[purcell]

This broke horribly because of 52acefd821768a73c38700f4bd399225433d1fc2: that was a follow-up commit I made to this PR, hoping to resolve byte compilation warnings the PR introduced. Namely, whole-line-or-region-bind-keys and whole-line-or-region-extensions-alist refer to each other, but if the former is placed after the latter, it causes a recursive load issue due to the ;;;###autoload cookie. I've put things back as they were, but there should be a better way to handle this, perhaps by simply removing the magic :set handler for the defcustom.

[thomasf]

whole-line-or-region-comment-dwim doesn't work at all for me anymore. It just creates a region from the beginning of the line to point and leaves it there.

[purcell]

@thomasf How are you testing that? By running M-x whole-line-or-region-comment-dwim? I note that whole-line-or-region-comment-dwim relies on whole-line-or-region-base-call, which did get changed as part of @andersjohansson's PR, but it's not obvious that any of these changes should have broken that command...

[thomasf]

I've tried that, and using my C-c ; binding..

(use-package whole-line-or-region
  :ensure t
  :commands (whole-line-org-region-mode)
  :bind (
         ("C-c ;" . whole-line-or-region-comment-dwim)
         ("C-w" . whole-line-or-region-kill-region)
         ("C-y" . whole-line-or-region-yank))
  :init
  (progn
    (define-key region-bindings-mode-map ";" 'whole-line-or-region-comment-dwim)
    ))

I also had a quick look at the recent commits yesterday and did not see anything obvious either, did not have time to look into it then.. not really now either but it was messing with my productivity all of yesterday so I might need to actually take a look at it..

[thomasf]

trying it out in an fundamental mode buffer confirms that it actually executes the commenting function because I go the prompt about "no comment syntax defined"

[thomasf]

the kill and yank bindings seems to be working properly now and I don't remember having any issues with them yesterday either.

[thomasf]

Right now, it looks like adding comments is failing while removing them works fine.

[thomasf]

btw. I ran out of time trying to understand the significant difference between the old and new code so I just replaced the source code in my elpa/ directory with a60e022b30c2f4d3118bcaef1adb77b90e0ca941 to get my environment working again, for now.