Closed martin-fleck-at closed 1 month ago
I'll take a thorough look at this tomorrow
Some usability observations first of all:
The 'tentative' selection state (orange outline) is a bit confusing. The 'full' selection state available after enter
seems fairly low risk, and I'm not sure that a 'preview' is necessary.
Keyboard navigation is broken when data is missing in one column:
Navigation works here:
Because the variable column is populated
But I can't get to the ASCII column from the data column in these rows:
because there's nothing in the Variables column here, and the logic assumes it will only jump one column.
Since we're allowing selection + copy, it would be nice to be able to use shift selection to select e.g. multiple groups or an entire row for copying.
Thanks, @martin-fleck-at ! Finally got around having a play with it. This is a nice enhancement! Some thoughts from using it
Styling issues which might have been there before, only spotted them now. Probably better to tackle via separate issue if no regressions:
Thank you for the review @jreineckearm and @colin-grant-work! I'll have a closer look at this tomorrow but just to clarify some things.
The 'tentative' selection state (orange outline) is a bit confusing. The 'full' selection state available after
enter
seems fairly low risk, and I'm not sure that a 'preview' is necessary.
This sounds like you want no highlighting of the currently focus cell (orange outline), is that right? I could imagine that might be quite confusing if you navigate more than one or two cells further. What exactly is confusing or maybe I'm not fully understanding your remark.
Keyboard navigation is broken when data is missing in one column:
Good catch! We need to check how far we can actually get to the right instead of giving up just after one jump I guess.
Since we're allowing selection + copy, it would be nice to be able to use shift selection to select e.g. multiple groups or an entire row for copying.
I'll check how much effort this might be since supporting the shift selection also kind of implies multi-select which in turn also implies the support of Ctrl+click selection of non-adjacent cell. Maybe it is quite straight-forward but if it is not, I might suggest pulling this out into a separate issue. What is your opinion of multi-select behavior @jreineckearm?
- Would it make sense to also enable Ctrl+V on the selected group without the need to enter editing mode via Space first? That would allow a sequence of navigate-to-group -> hit-enter -> Ctrl+C -> navigate-to-group -> hit-enter -> Ctrl+V -> navigate-to-next-group -> hit-enter -> Ctrl+V -> navigate ...
That seems like a reasonable enough request, I'll check how fast I can implement this and how it feels. I assume this shall only be available on single-select then, is that right? Just trying to consolidate the feedback ;-)
- After editing a group and confirming by Enter, I was not able to get back to navigating via cursor keys until a mouse-click on a group. Looks like the focus went on a different level, cursor-up/-down caused scrolling.
Interesting, I thought I had fixed that but will definitely double-check.
- Might be worth in future to combine the frame of the mouse-hover-over with the frame of the cursor navigation, last activity specifying the "owner". I see though the potential for confusing behavior, hence not fully convinced myself.
Yeah, that might make sense.
Styling issues which might have been there before, only spotted them now. Probably better to tackle via separate issue if no regressions:
- Dark (Visual Studio): The group selection highlight makes it hard to spot the character selection highlight in edit mode. I see that in a couple of other color themes as well (e.g. Monokai).
- Light (Visual Studio): Moving the mouse over groups makes text disappear (or font foreground and background color are the same). So far only spotted for this theme.
Will need to check on those, hopefully not too much work anyway.
This sounds like you want no highlighting of the currently focus cell (orange outline), is that right? I could imagine that might be quite confusing if you navigate more than one or two cells further. What exactly is confusing or maybe I'm not fully understanding your remark.
I think the confusion is that, while you refer to it as the 'the currently focused cell,' it doesn't behave as though it's the current focus: additional action is necessary to turn it blue, and so make it the actual focus. While it's orange, no action seems to apply to it: copy targets blue cells, editing turns the cell blue, etc.
I'm not sure if part of the implementation is based on the Theia extension's behavior. In that case, selection applied only to data columns, and there were only two selection states, highlighted (orangish) but not editing and editing. In this implementation, there are three selection states for data column groups, orange, blue but not editing, and editing, and two for everything else, orange and blue. I think in both cases, it's one state too many.
Good catch! We need to check how far we can actually get to the right instead of giving up just after one jump I guess.
I'd suggest that a lot of the index passing (row index, column index, group index) that the new code introduced may be off the mark. If we need to be dynamic anyway to account for the possibility of missing data (vertically, as well - variables can be sparse in the vertical direction), a combination of closest()
, index finding, and previous/next sibling might be just as effective, and a bit clearer, than query selectors based on index data properties.
@jreineckearm @colin-grant-work I pushed an update that addresses some of your concerns:
While I re-implemented the navigation with no data attributes, I did not yet remove the data attributes of the groups because I still believe they are useful, e.g., when restoring focus after an operation.
If you agree, I'd rather move multi selection out into a separate issue/PR.
I think with the new styling changes maybe the confusion about the states is mostly removed. I tried to mimic what is happening in the VS Code tree (e.g., file tree), where you have selection and can then start to navigate using the arrow keys while the "old" selection is still highlighted. However, as rightfully pointed out by Colin, any operations should target the focused cell and not the selected cell so copy is now handled on that level.
Tested it on macOS:
@arekzaluski Thank you for your feedback! Indeed, under MacOS the events need to be handled a bit differently due to the meta
key being the modifier instead of the ctrl
key. I resolved some merge conflict with the master and pushed an update that should also work under MacOS. However, I don't own a Mac so I'd appreciate it if you could take a look.
@arekzaluski Very curious, I cannot reproduce this on Linux and as far as I can see we do rely on the InputText from PrimeReact for that. We do stop propagation of the event but it should still be handled I believe. Maybe we should create a separate task for it.
@colin-grant-work Do you want to have another look at it or are you good with the changes so far?
What it does
Introduce group-level selection and navigation for table
Relates to https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/issues/106 but does not include quick navigation up and down with PageUp/PageDown/Home/End/etc keys.
How to test
Review checklist
Reminder for reviewers