davidshepherd7 / frames-only-mode

Make emacs play nicely with tiling window managers by setting it up to use frames rather than windows
GNU General Public License v3.0
168 stars 6 forks source link

Persistent enabling via customize interface does not seem to work #25

Open jsyjr opened 6 years ago

jsyjr commented 6 years ago

Fairly recent (nearly 26.1) version of emacs built from git:

GNU Emacs 27.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 2.24.31) of 2017-10-16

davidshepherd7 commented 6 years ago

From peeking at your config on github it looks like you are autoloading frames-only-mode, which (AFAIK) will only load it when the function frames-only-mode is called. So it's possible that custom-set-variables doesn't trigger the autoload.

I can only reproduce the issue with a similar setup:

(add-to-list 'load-path "/home/david/code/frames-only-mode/")
(autoload #'frames-only-mode "frames-only-mode" nil t)
(custom-set-variables '(frames-only-mode t))

if I replace the autoload with (require 'frames-only-mode) then it works.

Do you think that's the problem? I'm not familiar with using custom variables to turn on modes.

jsyjr commented 6 years ago

On Thu, Dec 28, 2017 at 7:05 AM, David Shepherd notifications@github.com wrote:

From peeking at your config on github it looks like you are autoloading frames-only-mode, which (AFAIK) will only load it when the function frames-only-mode is called. So it's possible that custom-set-variables doesn't trigger the autoload.

​I am impressed that you decided to locate my config. Thanks for that extra effort.

I can only reproduce the issue with a similar setup:

(add-to-list 'load-path "/home/david/code/frames-only-mode/") (autoload #'frames-only-mode "frames-only-mode" nil t) (custom-set-variables '(frames-only-mode t))

if I replace the autoload with (require 'frames-only-mode) then it works.

Do you think that's the problem? I'm not familiar with using custom variables to turn on modes.

​I can indeed get your package to work. That said there clearly were various things not quite right about it ​how it interacted with define-minor-mode, autoloading and the custom framework.

My first clue was that in its current form your package creates to customization groups: frame-only and frame-only-mode.

The frame-only group seemed to be created in define-minor-mode by stripping the trailing '-mode' from the first argument (frames-only-mode). That group contained a single entry: frames-only-mode. The description was the misleading default text supplied by define-minor-mode:

Non-nil if Frames-Only mode is enabled.
See the ‘frames-only-mode’ command
for a description of this minor mode.
Setting this variable directly does not take effect;
either customize it (see the info node ‘Easy Customization’)
or call the function ‘frames-only-mode’.

describe-variable on frames-only-mode returned the same text along with the additionally erroneous assertion:

You can customize this variable.

After comparing your code with other minor modes I have concluded that there is a pervasive convention that a feature and the mode that controls it typically do not share the same symbol. That is feature XYZ is enabled and disabled by the function XYZ-mode. Further the autoload convention is that the name of a file providing feature XYZ is XYZ.el.

Based on those observations I modified your frames-only-mode.el to use the string 'frames-only-mode' only when referring specifically to the feature control function or the variable declared by define-minor-mode to record the current state of the feature. In all other instances I replaced 'frames-only-mode' with 'frames-only'. I separated and corrected the documentation of the frames-only-mode function and the identically named variable. Finally I renamed frames-only-mode.el to frames-only.el.

Following those changes I have only a one customize group (frames-only) containing the following options:

 frames-only-configuration-variables
 frames-only-kill-frame-when-buffer-killed-buffer-list
 frames-only-reopen-frames-from-hidden-x11-virtual-desktops
 frames-only-use-window-functions
 frames-only-use-windows-for-completion"

(Note: no frames-only-mode option.)

Here is the function documentation:

frames-only-mode is an interactive autoloaded compiled Lisp function
in ‘frames-only.el’.

(frames-only-mode &optional ARG)

Use frames instead of emacs windows (e.g. for tiling window managers).

The following variables can be set or customized to configure behavior:
  frames-only-configuration-variables
  frames-only-kill-frame-when-buffer-killed-buffer-list
  frames-only-reopen-frames-from-hidden-x11-virtual-desktops
  frames-only-use-window-functions
  frames-only-use-windows-for-completion

And here is the variable documentation:

frames-only-mode is a variable defined in ‘frames-only.el’.
Its value is t

Documentation:
Non-nil if frames-only-mode is enabled.

See the ‘frames-only-mode’ command for a description of this minor mode.
Setting this variable directly or via customization has no effect.  The
only effective way to modify it is to call ‘frames-only-mode’.

With that this el-get recipe now works as expected:

(:name frames-only
     :description "Make emacs play nicely with tiling window managers

by setting it up to use frames rather than windows" ;; :type github ;; :pkgname "davidshepherd7/frames-only-mode" :type http :url "file://localhost/home/jyates/emacs/frames-only.el" :after (frames-only-mode t) :features frames-only)

Because I renamed your file I am attaching my modified version rather than suggesting a github pull request. Feel free to do with my edits as you choose. I did nothing truly creative in these edits so no need to give me credit if you choose to keep any of them.

Thanks for the nice package,

/john