Visidata: Error writing rstatus when repeating a command with no keystrokes

[ajkerrigan]

Small description
If the last command in the cmdlog has an empty keystrokes column, repeat-input throws TypeErrors trying to update the status line.

Expected result
Commands are repeatable whether they were issued with keystrokes or long names.

Actual result with screenshot
If you get an unexpected error, please include the full stack trace that you get with Ctrl-E.

 Traceback (most recent call last):
   File "/Users/akerrigan/code/visidata/visidata/statusbar.py", line 183, in drawRightStatus
     rstatus = ' '+rstatus
 TypeError: can only concatenate str (not "NoneType") to str

Steps to reproduce with sample data and a .vd
Please attach the commandlog (saved with Ctrl-D) to show the steps that led to the issue.
See here for more details.

vd sample_data/benchmark.csv
<space> select-after
<space> repeat-input

Trying to replay a .vd with these commands _won't_ show the error. Inside VisiData, the keystrokes value for select-after is None. That gets converted to an empty string when we save the command log. So we can only see the error in cases like repeating or redoing a longname command interactively.

Additional context
Please include the version of VisiData.

2.-4dev (70e837dbca12f4df56485a7a5da0a0483e78439b)

[saulpw]

Nice find, @ajkerrigan. There is of course an easy fix, but before we do that, I've had the thought many times that maybe the right status should show the longname instead of the keystroke (which it's been doing since before we had longnames). As long as we're here, what do you (and others if they're listening) think about that? Which would you prefer to see after each command? Or maybe both? Or neither?

[anjakefala]

@saulpw Keystrokes take up less space which may matter with smaller terminal sizes ... that is one thing to check out.

Beyond that, that might be a really good thing to do. It gives a much clearer idea of "this is the most recent action that happened". Not every command has a keystroke, but every command has a longname.

[ajkerrigan]

Ooh interesting idea :). My gut reaction is to want the long name only if there's no keystroke. I'm thinking seeing something like go-down (j) would be extra terminal space for little value, but always having _some_ indicator of the last action seems handy.

It could be a configurable option to always display the long name perhaps, but I can't tell if that's a useful option or a copout to avoid making a decision :).

[saulpw]

So, there is disp_rstatus_fmt, which you can add {vd.keystrokes} to, but that's redundant since it's displayed anyway. Why, you ask? Well keystrokes is factored out for two reasons:

• it has its own color (color_keystrokes instead of color_status)
• in split-sheet mode, only the active sheet shows the keystrokes--it felt weird when they both did. But you definitely want the other information in the right status bar (#rows and rowtype) to be displayed on the inactive sheet.

keystrokes is also not just the last keystroke pressed, nor just the keystrokes typed for the last command. It's the prefixes in the input buffer, so you can see the 'state' (since it's so simple). Then until the next keystroke is typed, it shows the last full set of keystrokes. vim does this too, except it doesn't let the keystrokes linger until the next keystroke.

So we can almost add {sheet.lastLongname} or something, if we are willing to define the lastLongname property. Left as an exercise for the user :)

All this is to say, I think we should not do this after all (and just take the simple fix). I had forgotten the nuances of vd.keystrokes, and so really the only option is the above, which I think I agree, should not be the default experience.

[anjakefala]

keystrokes is also not just the last keystroke pressed, nor just the keystrokes typed for the last command. It's the prefixes in the input buffer, so you can see the 'state' (since it's so simple). Then until the next keystroke is typed, it shows the last full set of keystrokes.

ohhhhh, right you are!

[ajkerrigan]

Ah, tricky business. A simple fix seems like a fine call. I could see what _looked_ like a few different simple/easy fixes but I'm not sure which makes the most sense or what other keystroke nuance I'm missing.

So we can almost add {sheet.lastLongname} or something, if we are willing to define the lastLongname property. Left as an exercise for the user :)

Out of curiosity I ran a test with a small change:

+++ b/visidata/basesheet.py                                                                                            [0/2582]
@@ -54,6 +54,7 @@ class BaseSheet(Extensible):
         self.source = None
         self.rows = UNLOADED      # list of opaque objects
         self._scr = mock.MagicMock(__bool__=mock.Mock(return_value=False))  # disable curses in batch mode
+        self.lastLongname = ''  # last command executed, for display

         self.__dict__.update(kwargs)

@@ -125,6 +126,8 @@ class BaseSheet(Extensible):
             code = compile(cmd.execstr, cmd.longname, 'exec')
             vd.debug(cmd.longname)
             exec(code, vdglobals, LazyChainMap(vd, self))
+            if cmd.longname != 'exec-longname':
+                self.lastLongname = cmd.longname
         except EscapeException as e:  # user aborted
             vd.warning(str(e))
             escaped = True

And in ~/.visidatarc:

options.disp_rstatus_fmt = "{sheet.lastLongname} {sheet.nRows:9d} {sheet.rowtype}"

I would assume that change is too optimistic or naive, but after a bit of fiddling I like the extra info. It was enough to make me take back my objection to wasted space displaying commands like "go-down". Seems like helpful context for demos/screensharing/onboarding too.

[saulpw]

Interesting. I think that's a fine approach and I'm glad that for once the naive change works. The only downside is the ongoing accretion of cruft in BaseSheet and TableSheet, so we should weigh whether this feature is ultimately worth the extra few lines of code. But if you say it's already impressed you, I may well give it a try. Maybe it will become the new hotness!

[saulpw]

Also see the previous commit in which I added sheet.longname which is now in rstatus by default to see how we like it.