Add buffer name for raw pane event viewer
#3791
Conversation
blamedrop
changed the title
raw pane event viewer
|
@dmaluka can you approve the workflow?
|
|
I don't know what is "workflow approval", but I can merge this PR right away, unless @JoeKar or anyone else has any objections. |
|
Short feedback from vacation:> 1 workflow awaiting approvalRefers to the automatic build and test workflow, which isn‘t performed without extra approval in PRs from new contributors.But this topic reminds me to:#3689 (comment) @cutelisp‘s #3791 (comment) isn‘t that bad and it be somehow „consistent“.Am 05.07.2025 um 22:09 schrieb Dmytro Maluka ***@***.***>:dmaluka left a comment (micro-editor/micro#3791)
I don't know what is "workflow approval", but I can merge this PR right away, unless @JoeKar or anyone else has any objections.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
But as I found out and reported in #3791 (comment) (ab)using fake path to provide name for "virtual" buffers also have some issues attached 🤷♂️ |
Turns out my suggestion can lead to sketchy events #3791 (comment). So I agree with @blamedrop it's seems better to use If this |
|
Ok…overread that example. So we should fix the log case with a second commit here too and then it can be merged. Thank you!Am 06.07.2025 um 03:35 schrieb cutelisp ***@***.***>:cutelisp left a comment (micro-editor/micro#3791)
@JoeKar isn‘t that bad and it be somehow „consistent
Turns out my suggestion can lead to sketchy events #3791 (comment). So I agree with @blamedrop it's seems better to use SetName instead of a hacky fake path.
If this SetName approach gets accepted here, we can still be consistent by changing existent similar buffer creations. e.g. https://github.com/zyedidia/micro/blob/41b912b5392ff80c1cce7d0ef7668f406977cc00/internal/action/globals.go#L14
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Using such fake path have some issues as mentioned in micro-editor#3791 (comment) comment
Done! |
|
Shall we adapt the info buffer as well?Am 06.07.2025 um 12:35 schrieb blamedrop ***@***.***>:blamedrop left a comment (micro-editor/micro#3791)
Ok…overread that example. So we should fix the log case with a second commit here too and then it can be merged.
Done!
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
WDYM? This one? ib.Buffer = buffer.NewBufferFromString("", "", buffer.BTInfo)It doesn't have 'fake path'. Would setting its name have any visible effect anywhere? |
LGTM
Don't think so, not sure why would we add a name to it |
|
Might not be the most obvious use case, but printing the name of the buffer within the log could help somewhere. And does it hurt to spend around 4-10 Bytes for this string? ;) Am 06.07.2025 um 14:12 schrieb cutelisp ***@***.***>:cutelisp left a comment (micro-editor/micro#3791)
Done!
LGTM
Would setting its name have any visible effect anywhere?
Don't think so, not sure why would we add a name to it
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
It doesn't hurt at all 🤕. I just said it because I don't think we were using the name anywhere. We can do it for a matter of consistency 👍. |
|
Right, good idea, having it named could be useful in debugging/logging. I'll do it later. By the way, since we're discussing it here quite extensively, what do you think about refactoring |
Indeed. This is problematic even just because it prevents us from opening a real file named
I'm not sure that would be very useful (if the point is to distinguish the infobuffer from other buffers, there are other ways to do that, e.g. print its But, yeah, it wouldn't hurt to do that, e.g.: --- a/internal/info/infobuffer.go
+++ b/internal/info/infobuffer.go
@@ -46,6 +46,9 @@ func NewBuffer() *InfoBuf {
ib.Buffer = buffer.NewBufferFromString("", "", buffer.BTInfo)
ib.LoadHistory()
+ // this name isn't really displayed anywhere, set it just for debugging purposes
+ ib.SetName("Info buffer")
+
return ib
}
Anyway, I think this PR is good to merge as is.
IMHO that would be an overkill. |
|
Okay, thanks. Feel free to merge it then. |
|
Approved Am 06.07.2025 um 18:08 schrieb blamedrop ***@***.***>:blamedrop left a comment (micro-editor/micro#3791)
Okay, thanks. Feel free to merge it then.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@dmaluka could you merge it? |
|
Could you rebase, to remove this merge commit: cad445e ? |
Uh, not really, I'm AFK, sorry. Don't you prefer squash merging anyway? It won't include that merge branch commit IIRC. |
Well, it isn't urgent, is it? ;)
You have two independent changes in two separate commits, so it would be nice to keep them separate in the git history. I only prefer squashing in cases when the commit history in the PR is a mess (and the author is too clueless to clean it up). |
|
Ok, just tried "Rebase and merge" and it seems to work. Done. :) |
|
Although AFAICS the downside of "Rebase and merge" is that there is no PR number added in any of the commit messages. (Looks like Github in its wisdom adds it only if either there is just one commit or there is a merge commit, not if there are 2 or more commits rebased without a merge commit.) |
|
Thank you!
True, that's unfortunate.
|
4 participants
Right now it has
No namelike every other new file.