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

[statusbar-] move sheet.longname into vd object #2186

Merged
merged 1 commit into from
May 24, 2024

Conversation

midichef
Copy link
Contributor

Immediately after moving to a new sheet, the sheet.longname in the statusbar is incorrect. On the latest 3.0dev as well as 2.11.1.

For example, right after sheets-stack, the longname displayed is blank. That's because the new sheet's sheet.longname is ''. Then if we quit out of that sheet with q, the longname shown in the original sheet is incorrect. I expect it to show "quit-sheet", but instead it shows "sheets-stack", the name of last command executed in the original sheet.

This PR moves longname into the vd object, instead of storing it in each sheet. Will this work?

And is it safe to remove BaseSheet.longname?

BaseSheet.init('longname', lambda: '')

I think it's now obsolete but it's possible I missed a use somewhere.

@saulpw
Copy link
Owner

saulpw commented Dec 23, 2023

Thanks for noticing and addressing this. I believe sheet.longname isn't used anywhere else, and could be removed with this change. However, I believe that this change means both windows in splitpane will show the same last command; the intent is that if you don't do anything in one of the panes, it will still show the last command executed on that sheet. It's a minor issue, and I don't think we need to preserve that behavior, so perhaps we shouldn't show the previous command on the inactive sheet at all. But we should make sure splitpane acts reasonably here too.

@midichef
Copy link
Contributor Author

Oh, now I see the reason why each sheet stores longname.

What if we copy sheet.longname from the active sheet to the next sheet, when doing vd.push() and vd.quit()?
Something like 2a7b8b0?

@saulpw
Copy link
Owner

saulpw commented Dec 25, 2023

What if we copy sheet.longname from the active sheet to the next sheet, when doing vd.push() and vd.quit()?

Does this work for both cases? If so I'm willing to try it out.

@midichef
Copy link
Contributor Author

midichef commented Dec 30, 2023

It works with a single pane and for split panes.

But it is a bit hacky, and push() is called in a few dozen lines where it's not in a command string in addCommand. I can imagine situations where push() gets called, and longname gets copied when it should not be. I looked the existing uses over and don't see any such situations, but I could have missed one. But even if it's not happening now, it seems likely to eventually happen.

The situation for quit() is safer. It is only called in 3 commands and I checked them all.

So maybe only quit() should copy longname, and push() should not.

@midichef
Copy link
Contributor Author

midichef commented Mar 1, 2024

I've replaced the original commit with the discussed (2a7b8b0) that keeps sheet.longname and carries it across push() and quit().

For future reference, here are all the uncertain calls to vd.push() that I thought about for this patch. And some notes.
push.json
The rest of the calls to vd.push() are all done directly in the addCommand() command string, or done in a function only called in the command string. So they should work fine.

It's a bit inelegant, but I think it's worth it, to help orient users. Sometimes I really want to see what command I just activated to create a new sheet.

@midichef midichef marked this pull request as ready for review March 1, 2024 02:35
@midichef
Copy link
Contributor Author

The drawback of my previous proposed fix is that push() and quit() can be called from contexts that are not a command string. Like plugins, or .visidatarc user code, or future changes to Visidata source code.

There is a better solution (e7da985). Instead of copying longname from the last sheet on push()/quit(), read it from activeCommand. Then status will only be set when push() and quit() are called from commands.

activeCommand has a threading problem at the moment, so it's safest to wait for a resolution on #2345. That's why I've placed e7da985 on the tip of that branch.

@midichef
Copy link
Contributor Author

I've rebased this patch onto current develop, so it's ready for review.

Copy link
Owner

@saulpw saulpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this, @midichef !

@anjakefala anjakefala merged commit d5ca442 into saulpw:develop May 24, 2024
13 checks passed
@midichef midichef deleted the status_longname branch May 25, 2024 09:41
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

Successfully merging this pull request may close these issues.

None yet

3 participants