zyedidia / micro

A modern and intuitive terminal-based text editor
https://micro-editor.github.io
MIT License
24.47k stars 1.16k forks source link

Does log pane must have one instance? #3098

Open dustdfg opened 7 months ago

dustdfg commented 7 months ago

Does log pane must have one instance?

https://github.com/zyedidia/micro/blob/2d82362a6695a7e898455ce016449167ac439ddd/internal/action/globals.go#L8-L9

log don't check if log buffer is open it just checks if you ran it from log buffer.

> log
Ctrl+w to switch to non logbuf
> log

It looks like a sorta bug. We have only one pointer for global LogBufPane so we must have only one log pane at the moment(?) Am I right it is a bug? @dmaluka @JoeKar

It is easy to correct for > log https://github.com/dustdfg/micro/commit/da2d4cf56d92a7fd51da5aed569a931a4b1bb2d0 but I am not sure what to do with

https://github.com/zyedidia/micro/blob/2d82362a6695a7e898455ce016449167ac439ddd/internal/action/globals.go#L30-L36

Does comment rely on the fact it is opened only by ToggleLogCmd?

JoeKar commented 7 months ago

Maybe I misunderstood something, but doesn't log simply store the commands used? In that case it can be seen as a simple singleton.

dustdfg commented 7 months ago

Yeah. It is singleton but you don't need many panes that show you the same thing. Look you have global log buffer

https://github.com/zyedidia/micro/blob/2d82362a6695a7e898455ce016449167ac439ddd/internal/buffer/buffer.go#L38-L40

When you create new log pane it just uses that buffer. So it doesn't matter how many log panes you will open they will refer the same buffer. You will see the same content...

We currently have only one pointer for log pane see previous comment. And when OpenLogBuf function is executed it creates new log pane and overrides the pointer that previously contained the pointer to previous log pane. Also when you close that log pane nothing sets the pointer to nil...

In general you will end with several log panes and with only one pointer. The panes will show the same thing because have the same buffer... It looks like a bug...

dustdfg commented 7 months ago

Also want to mention that this kind of singleton looks like it should be treated in a special way. I mean that if it is only one pane why we split currently active pane and open one small log pane? If log pane is something global it should be opened in the root... I meant that the following situation doesn't look good. screen-1702748152

If we have something global as log it should be opened always in root like here: screen-1702748207

P.S The more I know about log panes the more I feel that it should be treated like a special entity as infobar for example...

JoeKar commented 7 months ago

P.S The more I know about log panes the more I feel that it should be treated like a special entity as infobar for example...

Might be a feature for the future. :wink:

dmaluka commented 7 months ago

We could apply such a simple fix (in addition to https://github.com/dustdfg/micro/commit/da2d4cf56d92a7fd51da5aed569a931a4b1bb2d0):

--- a/internal/action/globals.go
+++ b/internal/action/globals.go
@@ -31,6 +31,8 @@ func WriteLog(s string) {
 // If the current bufpane is a log buffer nothing happens,
 // otherwise the log buffer is opened in a horizontal split
 func (h *BufPane) OpenLogBuf() {
-   LogBufPane = h.HSplitBuf(buffer.LogBuf)
+   if LogBufPane == nil {
+       LogBufPane = h.HSplitBuf(buffer.LogBuf)
+   }
    LogBufPane.CursorEnd()
 }

(and obviously fix the documentation of OpenLogBuf)

This should make the log buffer pane a singleton like the log buffer itself, which should at least fix annoyances like plugin list opening a new split log pane each time unless a log pane is currently active.

However, it is not so obvious if we really want the log pane to be a singleton.

  1. Remember that we have multiple tabs. If the log pane is already open, but not in the current active tab, then with our above simple fixes, the user will e.g. run plugin list and will see nothing (until he switches to the tab where the log pane is opened), which is even more confusing than an extra unneeded split log pane.
  2. Less importantly: even within the same tab, I can imagine the user wants to have multiple split log panes, if the log buffer is large and the user wants to see different parts of it in different panes simultaneously (via scrolling those panes accordingly).

Well, perhaps p.2 is a rather marginal use case and we can leave it out. But p.1 seems to be an important issue. So perhaps what we ultimately want is: multiple log panes (sharing with the same global log buffer), but no more than one log pane per tab. BTW that is what vim does with its help panes, AFAICS.

Or perhaps we should not deprive users (or plugins) of the possibility to open multiple log panes in the same tab (as we need to support multiple log panes anyway, for multiple tabs), just not make it the default behavior of the plugin command and so on?

In general you will end with several log panes and with only one pointer. The panes will show the same thing because have the same buffer... It looks like a bug...

The fact that they share the same buffer is ok per se. But the fact that they do it by sharing the Buffer struct (not by having separate Buffers and just sharing their SharedBuffer, like it is done for normal file buffers) looks like a bug indeed. This bug results in some maybe not very annoying, but still noticeable inconsistencies, e.g.: 1. moving a cursor in one log pane moves it also in another (inactive) log pane (but this other log pane does not scroll accordingly). 2. searching text in one log pane searches it also in another log pane (and if hlsearch is enabled, the search results are highlighted in both panes).

dustdfg commented 7 months ago

OK. Taking in the account all the above, I can see a solution that I would expect thought it restrict users:

  1. We need a patch that will store shared buffer globally and create new buffer from it each time log pane is opened (It seems to me it is several lines)
  2. var LogBufPane *BufPane should be moved from global world to Tab so each instance will have its own log pane (a bit more complex but also should be much changes)
  3. Check when open log pane (will stay almost the same as discussed patch but will just check another variable)
  4. Always open log pane globally in the root of the tab not by splitting current buffer screen-1702748207

or maybe just

  1. Completely delete var LogBufPane *BufPane because it is used in only one place https://github.com/zyedidia/micro/blob/2d82362a6695a7e898455ce016449167ac439ddd/internal/action/globals.go#L23-L28 Another place where you can find it is the place of creation. We can think it isn't used there. IT could be used by user/plugin but it will be rewritten when new log pane is created so no user/plugin will rely on that
  2. As was mentioned above simple patch for shared buffer

I can add one more reason against singleton log pane. You work with a big log and decided to execute command that prints something to log pane. After that you will accidentally find yourself in the end of buffer. Although it is currently also happens. When I was in start of log pane and performed plugin list it moved me to the end. But with global log pane it couldn't be avoided via several log panes...