unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: [elpa] externals/hyperbole faa8294 2/3: Redo hyperbole-mode; new {r} HyRolo search in match buffer
       [not found] ` <20210429035709.3DF2320D0F@vcs0.savannah.gnu.org>
@ 2021-04-29  6:00   ` Stefan Monnier
  2021-04-30  2:35     ` Robert Weiner
  2021-05-16 20:12     ` Robert Weiner
  0 siblings, 2 replies; 4+ messages in thread
From: Stefan Monnier @ 2021-04-29  6:00 UTC (permalink / raw)
  To: emacs-devel; +Cc: Bob Weiner

> +(defun enable-hyperbole-mode ()
> +  "Enable Hyperbole global minor mode."
> +  (interactive)

The `hyperbole-mode` command already offers that functionality, so
there's no point providing this as an interactive command.
I'd recommend you use a "--" in the name and keep that function "internal".
Also please use a "hyperbole-" prefix.

> +  ;; Activate hyperbole-mode
> +  (run-hooks 'hyperbole-mode-hook))

`define-minor-mode` runs `hyperbole-mode-hook` for you already, so this
`run-hooks` will cause the hook to be run twice in a row.

>    :keymap hyperbole-mode-map

This argument is redundant.

> +(if after-init-time
> +    ;; Initialize Hyperbole key bindings and hooks.
> +    (hyperb:init)
> +  ;; Initialize after other key bindings are loaded at startup.
> +  (add-hook 'after-init-hook #'hyperb:init t))
>  
>  ;; !! FIXME: Loading a file should not change Emacs's behavior but we
>  ;; need this here for awhile until can ensure Hyperbole users know to

I suspect the above code and the above comment should be swapped?


        Stefan




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [elpa] externals/hyperbole faa8294 2/3: Redo hyperbole-mode; new {r} HyRolo search in match buffer
  2021-04-29  6:00   ` [elpa] externals/hyperbole faa8294 2/3: Redo hyperbole-mode; new {r} HyRolo search in match buffer Stefan Monnier
@ 2021-04-30  2:35     ` Robert Weiner
  2021-04-30  3:20       ` Stefan Monnier
  2021-05-16 20:12     ` Robert Weiner
  1 sibling, 1 reply; 4+ messages in thread
From: Robert Weiner @ 2021-04-30  2:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Lidell Mats, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

Thanks, Stefan.  Will implement these updates.

On your last comment, we want Hyperbole initialization deferred until after
Emacs init time, so that looks right to me.

Regards,

Bob

On Thu, Apr 29, 2021 at 2:00 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > +(defun enable-hyperbole-mode ()
> > +  "Enable Hyperbole global minor mode."
> > +  (interactive)
>
> The `hyperbole-mode` command already offers that functionality, so
> there's no point providing this as an interactive command.
> I'd recommend you use a "--" in the name and keep that function "internal".
> Also please use a "hyperbole-" prefix.
>
> > +  ;; Activate hyperbole-mode
> > +  (run-hooks 'hyperbole-mode-hook))
>
> `define-minor-mode` runs `hyperbole-mode-hook` for you already, so this
> `run-hooks` will cause the hook to be run twice in a row.
>
> >    :keymap hyperbole-mode-map
>
> This argument is redundant.
>
> > +(if after-init-time
> > +    ;; Initialize Hyperbole key bindings and hooks.
> > +    (hyperb:init)
> > +  ;; Initialize after other key bindings are loaded at startup.
> > +  (add-hook 'after-init-hook #'hyperb:init t))
> >
> >  ;; !! FIXME: Loading a file should not change Emacs's behavior but we
> >  ;; need this here for awhile until can ensure Hyperbole users know to
>
> I suspect the above code and the above comment should be swapped?
>
>
>         Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 2414 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [elpa] externals/hyperbole faa8294 2/3: Redo hyperbole-mode; new {r} HyRolo search in match buffer
  2021-04-30  2:35     ` Robert Weiner
@ 2021-04-30  3:20       ` Stefan Monnier
  0 siblings, 0 replies; 4+ messages in thread
From: Stefan Monnier @ 2021-04-30  3:20 UTC (permalink / raw)
  To: Robert Weiner; +Cc: Lidell Mats, rswgnu, emacs-devel

> On your last comment, we want Hyperbole initialization deferred until after
> Emacs init time, so that looks right to me.

I meant that the "FIXME" comment seems to also apply to the
(if after-init-time ...) code that precedes it, so the code should
probably be moved to after the comment.

I'm not sure that's the case, tho, so if it is not, you may want to add
a comment explaining why this (if after-init-time ...) code is
"harmless" (i.e. doesn't change Emacs's behavior).


        Stefan




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [elpa] externals/hyperbole faa8294 2/3: Redo hyperbole-mode; new {r} HyRolo search in match buffer
  2021-04-29  6:00   ` [elpa] externals/hyperbole faa8294 2/3: Redo hyperbole-mode; new {r} HyRolo search in match buffer Stefan Monnier
  2021-04-30  2:35     ` Robert Weiner
@ 2021-05-16 20:12     ` Robert Weiner
  1 sibling, 0 replies; 4+ messages in thread
From: Robert Weiner @ 2021-05-16 20:12 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

Hi Stefan:

These fixes were committed awhile back: commit faa8294.  Have a look.  --
Bob

On Thu, Apr 29, 2021 at 2:00 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > +(defun enable-hyperbole-mode ()
> > +  "Enable Hyperbole global minor mode."
> > +  (interactive)
>
> The `hyperbole-mode` command already offers that functionality, so
> there's no point providing this as an interactive command.
> I'd recommend you use a "--" in the name and keep that function "internal".
> Also please use a "hyperbole-" prefix.
>
> > +  ;; Activate hyperbole-mode
> > +  (run-hooks 'hyperbole-mode-hook))
>
> `define-minor-mode` runs `hyperbole-mode-hook` for you already, so this
> `run-hooks` will cause the hook to be run twice in a row.
>
> >    :keymap hyperbole-mode-map
>
> This argument is redundant.
>
> > +(if after-init-time
> > +    ;; Initialize Hyperbole key bindings and hooks.
> > +    (hyperb:init)
> > +  ;; Initialize after other key bindings are loaded at startup.
> > +  (add-hook 'after-init-hook #'hyperb:init t))
> >
> >  ;; !! FIXME: Loading a file should not change Emacs's behavior but we
> >  ;; need this here for awhile until can ensure Hyperbole users know to
>
> I suspect the above code and the above comment should be swapped?
>
>
>         Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 2163 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-05-16 20:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210429035707.30958.16274@vcs0.savannah.gnu.org>
     [not found] ` <20210429035709.3DF2320D0F@vcs0.savannah.gnu.org>
2021-04-29  6:00   ` [elpa] externals/hyperbole faa8294 2/3: Redo hyperbole-mode; new {r} HyRolo search in match buffer Stefan Monnier
2021-04-30  2:35     ` Robert Weiner
2021-04-30  3:20       ` Stefan Monnier
2021-05-16 20:12     ` Robert Weiner

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).