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

[pyobj-] improve display of sets and simple objects #2331

Merged
merged 3 commits into from
Mar 10, 2024

Conversation

midichef
Copy link
Contributor

@midichef midichef commented Feb 23, 2024

This set of commits is aimed at making pyobj-expr more user-friendly. As it stands now, the results from looking at certain values are quite strange.
None: gives a sheet with no rows
set([1,2,3]): a sheet with no rows (because what is shown are its attributes, not its elements)
str(): a sheet with no rows, with the header text, which is different from the previous two cases
1 or True: a sheet of attributes, hard to make sense of:

attribute   value docstring
denominator 1     <type int>
imag        0     <type int>
numerator   1     <type int>
real        1     <type int>

I've added a new sheet type, PythonAtomSheet to hold None or a bool, a number, or an empty string. The goal is to improve the display. The change makes it easy to distinguish an expression value of None from "", due to the shown. And it shows bools/numbers in a simple 1x1 cell, instead of a 4x3 sheet of display attributes.

The new sheet also doesn't allow diving deeper into such simple objects. By contrast, the way visidata already works is, after pyobj-expr for a number 1, you can keep hitting Enter to dive in on the denominator field infinitely. It's quite confusing for a new user.

And I fixed a bug where if a sheet had no rows, open-row takes the user to a new PyobjExpr sheet.
(to reproduce, vd nonexistent_file.tsv then ENTER)

Some quirks of this PR:

  1. None is shown as it is in a standard sheet, an empty cell with next to it. Maybe it should show the text "None".
  2. If you view bools, the call to deduceType will put ? next to the column header.
  3. The empty string goes into PythonAtomSheet unlike regular strings that go into a TextSheet. I did that to prevent further dives into the empty string. But that creates an inconsistency, the header for empty strings is "value", but for all other strings it's "text". To get rid of the special handling for empty strings, just change the code to:
        if pyobj is None or isinstance(pyobj, numbers.Number):
...
        elif isinstance(pyobj, str):
            return TextSheet(*names, source=pyobj.splitlines() if pyobj else [''])
        elif isinstance(pyobj, bytes):
            s = pyobj.decode(cls.options.encoding).splitlines()
            return TextSheet(*names, source=s if s else [''])

@midichef midichef force-pushed the pyobj_display branch 2 times, most recently from cf9bae5 to 0c4789b Compare February 23, 2024 11:27
@@ -102,7 +102,7 @@ def nextRow(sheet, n=1):
SheetsSheet.addCommand('gC', 'columns-selected', 'vd.push(ColumnsSheet("all_columns", source=selectedRows))', 'open Columns Sheet with all visible columns from selected sheets')
SheetsSheet.addCommand('z^C', 'cancel-row', 'cancelThread(*cursorRow.currentThreads)', 'abort async thread for current sheet')
SheetsSheet.addCommand('gz^C', 'cancel-rows', 'for vs in selectedRows: cancelThread(*vs.currentThreads)', 'abort async threads for selected sheets')
SheetsSheet.addCommand('Enter', 'open-row', 'dest=cursorRow; vd.sheets.remove(sheet) if not sheet.precious else None; vd.push(openRow(dest))', 'open sheet referenced in current row')
SheetsSheet.addCommand(ENTER, 'open-row', 'dest=cursorRow; vd.sheets.remove(sheet) if not sheet.precious else None; vd.push(openRow(dest))', 'open sheet referenced in current row')
Copy link
Owner

Choose a reason for hiding this comment

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

We're heading more towards using prettykeys like Enter for keybindings, and deprecating the ENTER variable (though we haven't officially done so yet). So let's not change these instances.

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.

Let's revert the changes to use ENTER for the keybinding, and then this is good to merge. Thanks @midichef!

@midichef
Copy link
Contributor Author

midichef commented Mar 8, 2024

Okay, I reverted the ENTER change, then consolidated the others. This PR is done.

@anjakefala anjakefala merged commit 1dd009a into saulpw:develop Mar 10, 2024
13 checks passed
@midichef midichef deleted the pyobj_display branch March 11, 2024 02:51
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