Skip to content

Conversation

@philtweir
Copy link
Contributor

@philtweir philtweir commented Jul 28, 2024

What does this PR do?

Adds simple navigation to the inspector, to explore a session starting from a single command. This creates a new user flow, where a user can find a history entry in the interactive view (in, say, Global mode), and hit Ctrl+o to navigate back and forward through that command's session.

IMAGINED USE-CASE: I remembered that I did a sequence of git steps but I can't remember the order and forgot to document it. I remember that reflog was involved and want to see the actual sequence, and only those commands.

IMAGINED USE-CASE: I used a curl command to get my IP address for greenlisting before I connected to the bastion server abc.xyz over SSH - I could easily find the SSH command with abc.xyz, and go back one step in the session, but without this change, scrolling through all my curl commands ever run to find a forgotten URL/domain would be too much work.

Since this gives the inspector tab a broader purpose than viewing analytics, it needs to function even when there are not enough screen rows for charts -- hence, this PR also introduces an ultracompact mode for the inspector that just shows the neighbouring history commands (as simple scrolling three-entry list, with no panes) if there are fewer than auto_hide_height rows (default: 8). Otherwise, the inspector behaves as normal, except that Up / Down will change the focused command by navigating through the session. That means there is no "compact" mode for the inspector - when the interactive search is compact (but not ultracompact), the inspector shows its usual chart view.

The UX for this could be improved - to keep this PR as lean as it realistically can be, I have tried to keep the flow very minimal, but a follow-up PR could introduce some tooltips, nicer ultracompact formatting, etc.

A minor QoL improvement that comes with this - since I had to deal with bold text and would otherwise have need a theming exception, I took the opportunity to ensure the theme engine sets styles completely (so a theme can have bold), not just colours. To limit scope creep, I do not add TOML syntax so (for now) you can only customize colours from config files, but it means that default-bold text (etc.) can now use the theming engine if the code-defined default Meaning is bolded.

Key changes:

  • introduces a simplified inspector tab, with only previous-current-next commands as rows, in compact mode
  • allows navigation through session history within the inspector (so compact inspector view is still useful)

It also (see comments below):

  • makes compact into compactness, an enum (to better standardize across inspector/interactive)
  • makes the inspector only change layout for ultracompact mode, which is still compact+(height<8)
  • clippy's complexity limit wanted draw split up a little, so not sure if this is a reasonable minimal way to do so for now
  • adds a (none) theme to the theming to enable output testing without styling
  • additional tests, although keen for input on how best to do these one functional test, as a starting point
  • documentation minor doc changes only, as I am not sure there is much to say

Was stacked on #2357, which is now in main

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

@ellie
Copy link
Member

ellie commented Aug 5, 2024

Generally all good, thank you! Ultra-compact works nicely.

However the inspector might need some adjusting. I run compact mode all the time (as do some others), and it will likely become the default in the future. In compact mode, the inspector looks like this regardless of size

CleanShot 2024-08-05 at 14 38 03@2x

I think it would make sense to continue to show the stats/etc if the terminal is large enough, but I do really like the scrolling through history/session.

Do you think you'd be able to split the inspector changes into a separate PR? Other than those commits, I'm happy to merge this as-is

@philtweir
Copy link
Contributor Author

Yep, sure! Good point - thinking about your comment, I realise I haven't properly thought about that stylistically-compact vs limited-space difference, so could I suggest I do two new PRs, if that makes sense:

  • inspector in ultracompact mode (so compact and <8 lines), including the interactivity (as otherwise it's a bit redundant)
  • inspector in compact mode, to start the discussion about what would be useful in general to change/keep/rearrange when compact is on (as I think you & others would need to input first to make sure I go in a sensible direction)

@ellie
Copy link
Member

ellie commented Aug 5, 2024

That sounds good to me!

@philtweir philtweir force-pushed the compactify-inspector branch from e7159ed to 64869b3 Compare August 10, 2024 09:08
@philtweir
Copy link
Contributor Author

I have split out #2357 as requested, which this PR is now stacked on. It has been updated to:

  • make compact into compactness, an enum (to better standardize across inspector/interactive)
  • made the inspector only change on ultracompact mode, which is still compact+(height<8)
  • clippy's complexity limit wanted draw split up a little, so not sure if this is a reasonable minimal way to do so for now

The intention (as above) would be to open a comment/forum-post to work out behaviour for a new PR for the inspector in (non-ultra-) compact mode, but this PR no longer has any change to the inspector except in ultracompact mode.

@philtweir philtweir changed the title RfC: Ultracompact Mode Ultracompact Mode for Inspector (was RfC: Ultracompact Mode) [stacked on #2357] Aug 10, 2024
@philtweir philtweir marked this pull request as draft August 10, 2024 09:15
@philtweir philtweir force-pushed the compactify-inspector branch from 64869b3 to 33c6f53 Compare August 24, 2024 10:04
@philtweir philtweir force-pushed the compactify-inspector branch from 33c6f53 to 55f5da5 Compare October 20, 2024 16:53
@philtweir philtweir force-pushed the compactify-inspector branch from 229d5e9 to d5cf9aa Compare December 29, 2024 12:40
@philtweir
Copy link
Contributor Author

Updated with a test - this turned out to be the hardest bit of the whole process, as the only sensible way I could see of testing a layout switch like this was to compare the output with TestBackend::assert_buffer_lines, but I could not for the life of me get it to match, likely due to ANSI code placement and how buffer area is calculated in different places in Crossterm/Ratatui :/

However, this forced me to take the PR beyond the inspector to enable a more sensible approach, by tweaking the new theming module -- this makes it possible to do the comparison without any styling. Obviously, this means the ultracompact-inspector styling is not checked, but the rest of the test is essential a functional test of ultracompact inspector layout, which is what I really wanted, rather than a brittle integration test tied to the default theme behaviour.

The downside is that I needed to add a new in-built theme called (none) (parentheses to highlight it is special, not general purpose). This only ever returns ContentStyle::default() so nothing is ever styled up, regardless of the Meaning passed. This might be separately useful where ANSI codes are not desirable or a theme author wishes to avoid inheriting anything from default now or in future, they could name (none) explicitly as a parent theme.

There is not much to put in the docs, but I have updated that PR too, so marking this ready-for-test.

@philtweir philtweir marked this pull request as ready for review December 29, 2024 14:52
@philtweir philtweir changed the title Ultracompact Mode for Inspector (was RfC: Ultracompact Mode) [stacked on #2357] Ultracompact Mode for Inspector (was RfC: Ultracompact Mode) Dec 29, 2024
@philtweir
Copy link
Contributor Author

philtweir commented Dec 29, 2024

As a side-note, to make boldness a property the theming can control, it also adds StyleFactory::from_fg_color_with_attributes so that in-built themes can pass Crossterm Attributes as well as a foreground colour. Given agreement on syntax, this could be exposed to the theming YAML (as could background properties). Would be keen to know if that is desirable first before proposing changes.

@atuin-bot
Copy link

This pull request has been mentioned on Atuin Community. There might be relevant details there:

https://forum.atuin.sh/t/feedback-playing-with-inspector/218/6

@philtweir
Copy link
Contributor Author

Rebased, as #2657 came up again. Wondering if this could be further split to make it more mergeable, or is it OK as-is?

As a side-note, I could see the inspector compact view going a different direction in future, so not hugely wedded to this implementation, but it seems like a reasonable stepping stone, at least?

@philtweir philtweir force-pushed the compactify-inspector branch from 7310c0d to b530df4 Compare April 20, 2025 14:23
@philtweir
Copy link
Contributor Author

Actually, one way to split this up would be to add the interactivity for the inspector in one PR, and then the ultracompact inspector in a second 🤔

@philtweir philtweir force-pushed the compactify-inspector branch from b530df4 to df5de10 Compare April 20, 2025 14:39
@philtweir
Copy link
Contributor Author

philtweir commented Sep 21, 2025

Just to confirm, the navigation works in both ultracompact and normal modes.

There is no special "compact" mode so it is always ultracompact (if below auto_hide_height) or normal, so in the case where the inspector has too few lines to show anything usefully, but not so low as your auto_hide_height , you just get a not very useful inspector window. But that is the case currently, anyway.

Screencast.From.2025-09-21.13-36-15.mp4

@philtweir philtweir changed the title Ultracompact Mode for Inspector (was RfC: Ultracompact Mode) Interactive Inspector Sep 21, 2025
@philtweir philtweir force-pushed the compactify-inspector branch from 5a88cfd to 5c21f71 Compare October 19, 2025 18:25
@philtweir
Copy link
Contributor Author

(Rebased on main)

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

ok! super happy with this, thank you! really appreciate your work and patience here <3

@ellie ellie merged commit d53ad84 into atuinsh:main Oct 20, 2025
19 checks passed
@ltrzesniewski
Copy link
Contributor

A little comment:

I had to quickly review this, and I think there's currently no way to reach InputAction::AcceptInspecting with accept being true. No big deal though I suppose.

@philtweir
Copy link
Contributor Author

Thanks @ltrzesniewski - I will take another look, it might be something I missed while rebasing!

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.

4 participants