Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling corfu-global-mode during startup #20

Closed
gusbrs opened this issue May 4, 2021 · 21 comments
Closed

Enabling corfu-global-mode during startup #20

gusbrs opened this issue May 4, 2021 · 21 comments

Comments

@gusbrs
Copy link

gusbrs commented May 4, 2021

Hi @minad !

I'm having a hard time trying to enable corfu-global-mode during startup. (Yes, it has come to that! ;-)

The apparent behavior is, right after startup, though I have enabled corfu-global-mode (I tried several alternatives, running the function, customizing it, even running it after-init-hook), corfu does not actually kick in on my scratch buffer. If I run normal-mode (my initial major-mode is Org, but it does not matter here), or visit a file, then it is there.

As far as I understand, the problem seems to be that I start Emacs with the usual emacsclient --create-frame --alternate-editor="", and the test for graphical display into corfu-mode fails during startup in these circumstances. Indeed, immediately after startup I find in the message buffer:

Corfu mode is only compatible with the graphics display. [5 times]
...
Corfu mode is only compatible with the graphics display. [2 times]

Even running (add-hook 'after-init-hook #'corfu-global-mode) seems not to be late enough:

Corfu mode is only compatible with the graphics display. [4 times]

@minad
Copy link
Owner

minad commented May 4, 2021

Okay, but (display-graphic-p) returns non-nil at some point later on? Take a look at https://emacs.stackexchange.com/questions/24609/determine-graphical-display-on-startup-for-emacs-server-client

@gusbrs
Copy link
Author

gusbrs commented May 4, 2021

Yes, I'm starting a GUI Emacs client frame there. As I've said, immediately after startup, if I just run normal-mode it works (thus (display-graphic-p) returns non-nil at that point).

@gusbrs
Copy link
Author

gusbrs commented May 4, 2021

Take a look at https://emacs.stackexchange.com/questions/24609/determine-graphical-display-on-startup-for-emacs-server-client

I know how to "make it work" if I want to. This is not a "support request", but a "bug report"...

The question is then: shouldn't corfu handle the situation? That's all.

@minad
Copy link
Owner

minad commented May 4, 2021

I see, I don't think Corfu should handle this. Corfu needs some way to detect if the display is graphic or not. I could perform the check later, but I prefer if Corfu stays completely out of the completion game when running on the terminal. As you see, Corfu does not replace the completion-in-region-function and does not add any hooks in that case.

If you have a concrete proposal how the behavior could be improved I may reconsider. But this seems to be a relatively common problem and the correct solution seems to be the after-make-frame-functions.

@minad minad closed this as completed May 4, 2021
@gusbrs
Copy link
Author

gusbrs commented May 4, 2021

I see, I don't think Corfu should handle this. Corfu needs some way to detect if the display is graphic or not. I could perform the check later, but I prefer if Corfu stays completely out of the completion game when running on the terminal. As you see, Corfu does not replace the completion-in-region-function and does not add any hooks in that case.

Ok, thanks for considering.

If you have a concrete proposal how the behavior could be improved I may reconsider.

No, not really.

But this seems to be a relatively common problem and the correct solution seems to be the after-make-frame-functions.

I'm fine with that on my side. ;-)

Thanks!

@minad
Copy link
Owner

minad commented May 5, 2021

@gusbrs
Copy link
Author

gusbrs commented May 5, 2021

As you see fit for corfu, @minad .

It seems to me like a sensible way to do it.

@minad
Copy link
Owner

minad commented May 5, 2021

Maybe, I guess my sentiment to add code to handle this use case seamlessly would be different if I would use emacsclient myself. I only use it when calling back into emacs from shell-mode or term-mode. Generally I think it is a better approach to not add special code for setups which deviate heavily from the vanilla emacs configuration. People who use special setups are perfectly capable of doing the necessary adjustments themselves. You may have seen my Consult package - for this package it would be difficult to perfectly support EXWM or mini-frame setups, both are packages/setups which I am not using and which I cannot test without serious effort.

@gusbrs
Copy link
Author

gusbrs commented May 5, 2021

I personally wouldn't call using emacsclient "special setup". From noobs to oldtimers, the advice runs the same. Some like it, some don't, of course. It is still a judgment call from the package's perspective, and I do get your point. If I may, if you do refrain from adding one such check, I'd say a note about this in the readme would be welcome, since the "recommended" setup does not work for a very common use case.

Edit: Indeed, it appears more people use emacsclient than Emacs in the terminal https://emacssurvey.org/2020/

@minad
Copy link
Owner

minad commented May 5, 2021

Okay, I just gave emacsclient another brief try. I cannot help myself but it always leaves me underwhelmed. Since you seem to be an emacsclient proponent, how can I achieve the following?

  1. I want to have a single "emacs" command (shell script). The script should start a new emacs daemon and create a new frame, when no server is running.
  2. The emacs script should not wait but go immediately to the background in any case. It should not make any noise.
  3. It must be allowed to run the script without arguments, emacsclient usually complains with "file name or argument required".
  4. When emacs is already running and executing the script without argument, the already existing emacs frame should be raised and made visible.
  5. When emacs is already running and executing the script with a file argument, the file should be opened in the existing frame.

If I can achieve this, I think I may switch my setup over to use emacsclient. Maybe I am always do something wrong, but it seems that I always hit some roadblocks. I am sure it is doable with some amount of fiddling, which I am usually willing to do (Otherwise I would not write packages). But as you have seen with these overlays/child frame bugs, my patience with Emacs is not endless 🤷

@gusbrs
Copy link
Author

gusbrs commented May 5, 2021

Since you seem to be an emacsclient proponent [...]

Not a proponent, just an user. I'm a proponent that each one uses Emacs as they prefer.

If I can achieve this, I think I may switch my setup over to use emacsclient. Maybe I am always do something wrong, but it seems that I always hit some roadblocks. I am sure it is doable with some amount of fiddling, which I am usually willing to do (Otherwise I would not write packages).

I don't see any relation between this issue and you having to personally switch to emacsclient. You probably shouldn't change anything if you are satisfied with your workflow. At least not because of this. Unless you do want to give emacsclient a try for other reasons.

[...] how can I achieve the following?

The --create-frame/-c allows you to run it even if the file argument is missing (I don't know why it bitches at us in this case without that flag, I wasn't even aware it did so until now).

EmacsWiki has one script that seems to have the same kind of requirement you describe (https://www.emacswiki.org/emacs/EmacsClient, section "Nifty shell function for hassle-free starting of emacsclient"). I'm not sure it meets all your requirements, but it definitely seems overly complicated to me.

I believe you could also get the same kind of control for the existing frame on the Lisp side too, and pass it as --eval option to emacsclient. I do something of the sort for mu4e which, for some reason, does get confused if it is visible in two different frames at the same time.

But what I do think is that you are trying to reproduce a workflow to meet some restrictions which have more importance when you are running Emacs without the daemon, than when you run with it. The daemon/client structure adds one extra level of abstraction which you could, to a degree, leverage otherwise, but just becomes much more convenient. When running just emacs your "session" is tightly tied with your "frame". True, you can create new frames on top of that, but it is a child frame. With emacsclient those things are completely independent. You can have as many frames you want, and also none of them, the session is still there. So, when you create a new frame with emacsclient all of your data from the active server comes along: buffer list, variables etc, etc. In a way, this new frame is just a clone of the one you are trying to recover. I personally just create and delete frames at will (the later with C-x C-c, which is more convenient than the C-x 5 keys).

Naturally, some data hangs on the frame, like the window configuration. When I want to recover that, I'd resort to the WM (and yes, I'm one of those uncool kids whose WM steals M-TAB for this and just deprives corfu of its righteous binding :-). Also, because it is so easy to create/delete frames, I delegate part of this sort of tasks to the WM: I rarely keep any complex window configuration in an Emacs frame, I'd rather normally just open multiple frames and tile them through the WM.

Of course, much of this is very much a matter of taste (and muscle memory). All I'm saying is that perhaps you are trying to bind to a restriction you might not need to.

But as you have seen with these overlays/child frame bugs, my patience with Emacs is not endless

I hope none of my reports here have tested your patience in the least. If any of them struck you that way, this was unintended. As a matter of fact, had you not requested active reporting, I'd be watching corfu with much interest, but at a distance, as I had intended initially. I only did otherwise in the hope of being helpful.

@minad
Copy link
Owner

minad commented May 5, 2021

I don't see any relation between this issue and you having to personally switch to emacsclient. You probably shouldn't change anything if you are satisfied with your workflow. At least not because of this. Unless you do want to give emacsclient a try for other reasons.

Of course, there is no relation. I am just trying things out from time to time to see if it would work out for me. An in order to judge better how a feature should be supported, I have to try it.

I hope none of my reports here have tested your patience in the least. If any of them struck you that way, this was unintended. As a matter of fact, had you not requested active reporting, I'd be watching corfu with much interest, but at a distance, as I had intended initially. I only did otherwise in the hope of being helpful.

No, or at least it was not your fault. I must admit, I got a bit annoyed with Emacs when neither overlay nor child frame worked as I had intended. But this is purely my fault or the fault of some buggy Emacs implementation. Regarding x-gtk-resize-child-frames it is not even the fault of Emacs but probably the fault of Gtk itself. However Emacs could have implemented the workaround I am implementing inside the display engine itself. Please continue to report. As a matter of fact your reports improved the quality greatly! I am sorry for being snarky.

Regarding the work around we are discussing here, my point still stands. If I implement a work around it will either print the "no display available" message when pressing TAB which is worse or it will not print any error message at all as in the case of the company-box workaround. I prefer to keep the display check and the error message. Unfortunately this comes at the cost of the problematic behavior for the scratch buffer of emacsclient users. They are more than I had expected (40% according to the stats you posted).

@minad
Copy link
Owner

minad commented May 5, 2021

I guess the nifty emacs startup script would cover my use case mostly. However I have an issue with emacsclient and the i3 window manager, which ignores raise-frame. This would force me to use multiple frames or to switch manually to the reused frame. Given this caveat I can just stay with my workflow.

I usually keep a single Emacs instance and frame around and do most things from inside Emacs. You are right that the one-frame restriction is artifical. But I simply don't see a benefit in having multiple equivalent Emacs frames around. Some people use a one window per frame configuration, this makes more sense to me (you seem to be using Emacs a bit like this?). But I still wonder why one should prefer the window manager over the Emacs window management.

@gusbrs
Copy link
Author

gusbrs commented May 5, 2021

Regarding the work around we are discussing here, my point still stands. If I implement a work around it will either print the "no display available" message when pressing TAB which is worse or it will not print any error message at all as in the case of the company-box workaround. I prefer to keep the display check and the error message. Unfortunately this comes at the cost of the problematic behavior for the scratch buffer of emacsclient users. They are more than I had expected (40% according to the stats you posted).

I wouldn't even call it a workaround, just taking care of an existing use case. But it is your call, I have nothing further to add here.

But I simply don't see a benefit in having multiple equivalent Emacs frames around. Some people use a one window per frame configuration, this makes more sense to me (you seem to be using Emacs a bit like this?). But I still wonder why one should prefer the window manager over the Emacs window management.

Pretty much that's what I do, yes. It's just one way to do it. For me it makes sense because there are other things around besides Emacs, so when it comes to managing windows (non Emacs parlance) I can keep the bindings consistent for Emacs and the rest. The one-window-per-frame works pretty well once each frame shares the overall environment. Of course, some things Emacs window management does better, but I can always resort to that at any time too. It is just not the default, it's called when something particular is required.

@minad
Copy link
Owner

minad commented May 5, 2021

I wouldn't even call it a workaround, just taking care of an existing use case. But it is your call, I have nothing further to add here.

Okay, how should this be resolved then? It should somehow behave like this:

  1. For text-mode an error message should be shown on mode activation, and corfu should not activate itself.
  2. There should not be an error message when pressing TAB in text-mode. The error should be shown only once at startup.
  3. When starting via emacsclient, no error message should be printed. However you can also use emacsclient in text mode? How can that be handled?

I think putting corfu-global-mode into an after-make-frame-functions hook is not that bad.

@minad
Copy link
Owner

minad commented May 5, 2021

I pushed a fix. Please give it a try. Hopefully it works in most scenarios.

@gusbrs
Copy link
Author

gusbrs commented May 5, 2021

I pushed a fix. Please give it a try. Hopefully it works in most scenarios.

Thank you.

I haven't tested it yet, but I took a look at it and it seems a clever way to do it. You may deem it enough or not, let me elaborate.

However you can also use emacsclient in text mode? How can that be handled?

Indeed it can. And, thinking this through, it gets more complicated. The daemon is neither GUI nor terminal, and you can have, for the same server, a terminal frame and a GUI one (or more, of course).

Making corfu--completion-in-region a no-op for the terminal and issue the message upon call, as you did in the commit, is a way to handle this, but this fails "2" on your list, which I agree is a problem.

I think putting corfu-global-mode into an after-make-frame-functions hook is not that bad.

As far as I get, the comment above suggests after-make-frame-functions is actually unavoidable. The question being if corfu should handle it internally or if just an improvement in the README would suffice.

In the later case, I would probably not hook it directly, since I would only want to call the mode once, like so:

(defun my/corfu-global-mode-enable ()
  (corfu-global-mode 1)
  (remove-hook 'after-make-frame-functions #'my/corfu-global-mode-enable))

(add-hook 'after-make-frame-functions #'my/corfu-global-mode-enable)

But even that is not fully general, and I'd only go for it because I know I only use the GUI.

Also, I'm failing to see a way to avoid entirely a message on calling completion. Consider the following case. We have a daemon running and an open terminal frame connected to that server. We now call emacsclient and ask it to open a GUI frame, all the tests will pass on after-make-frame-functions, the global mode gets enabled. Now we go back to the terminal frame, visit a new file and call completion, what happens?

Or, considering the commit. You did just the previously described in the terminal, got your undesired message and corfu-mode disabled in the buffer. Now you go back to the GUI frame, switch to that buffer, will corfu work then?

I admit this is an extremely stretched use case (if it deserves that name), I'm just considering the possibilities.

@gusbrs
Copy link
Author

gusbrs commented May 5, 2021

I think I have a suggestion. I'm not sure you'll like it, but do bear with me.

Let the mode (global or otherwise) be enabled regardless of graphic display when the daemon is running, but make corfu--completion-in-region call the regular completion function if not on graphical display. This is pretty much what you did, but let it be silent, and don't disable the mode. Issue a warning only if corfu-(global-)mode is called interactively, and only refrain from enabling the mode in this case if we are on the terminal and the daemon is not running. I think this covers cleanly all the cases without special tinkering, or resort to after-make-frame-functions. The no-op for the terminal case can figure prominently in the documentation. WDYT?

Of course, feel free not to like it. ;-)

minad added a commit that referenced this issue May 6, 2021
@minad
Copy link
Owner

minad commented May 6, 2021

Yes, I knew that this will fail the mixed text+gui use case. This is all not good and I hope it gets obvious by now why I was hesitant to handle this in Corfu. I am torn somehow again. I considered to just keep the mode always enabled and always print a message when pressing TAB in non-graphics mode. A text-mode only user will still get an error message when trying to use Corfu. However it annoy users who wants to use the same configuration for text and ui and is okay with the single error message at startup. The whole point of this package is to be small and simple, so I don't want to add a check for interactive calls to the mode.

minad added a commit that referenced this issue May 6, 2021
… on TUI (See #20)

* When using the text-mode UI, fall back to the default value of
  the `completion-in-region-function'
* Since there are mixed setups with daemons/text/gui, don't print
  an error message but try to do the right thing in any case
minad added a commit that referenced this issue May 6, 2021
… on TUI (See #20)

* When using the text-mode UI, fall back to the default value of
  the `completion-in-region-function'.
* Since there are mixed setups with daemons/text/gui, don't print
  an error message but try to do the right thing in every case.
@gusbrs
Copy link
Author

gusbrs commented May 6, 2021

This is all not good and I hope it gets obvious by now why I was hesitant to handle this in Corfu. I am torn somehow again.

The more I think of it, the more I'm convinced that the check should be done when completion is called, not when the mode is enabled. As you did. And, in this case, indeed issuing a warning at that time might be a problem for some people. At this point, my view is that this is the only sane way to handle it. I hope you do reconcile with it eventually. :-)

I considered to just keep the mode always enabled and always print a message when pressing TAB in non-graphics mode. A text-mode only user will still get an error message when trying to use Corfu. However it annoy users who wants to use the same configuration for text and ui and is okay with the single error message at startup.

I think the missing warning on enabling the mode for the terminal case is not particularly troubling. It is not like the mode gets enabled by itself.

All in all, thank you very much once again!

@gusbrs
Copy link
Author

gusbrs commented May 6, 2021

Also, now I took it for a spin here, and it's looking good to me.

corfu-global-mode works as expected during startup for emacsclient. I could even have the same buffer opened at the same time at GUI and terminal, and corfu did as expected in each case.

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants