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

View arbitrarily long multi-line annotations for git tags #1138

Closed
skyfaller opened this issue Feb 18, 2022 · 12 comments · Fixed by #2120
Closed

View arbitrarily long multi-line annotations for git tags #1138

skyfaller opened this issue Feb 18, 2022 · 12 comments · Fixed by #2120

Comments

@skyfaller
Copy link

Is your feature request related to a problem? Please describe.
Happily, we can view git tag annotations, including annotations with multiple lines. Sadly, if the annotation is long and goes on for many lines, we can't see the whole thing. In my current terminal emulator, gitui only displays the first 23 lines.

This issue will increase in importance if we support creating multiple-line annotations for git tags.

Describe the solution you'd like
Adding a scrollbar seems like a good solution to me.

If adding scrollbars to a "popup window" is not desirable, perhaps refactoring the UI so that annotations can be viewed in a multi-panel interface, like the files list, might work better.

Additional context
Screenshot of gitui with a long git tag annotation and no way to scroll to read the rest of it

@skyfaller skyfaller changed the title Read arbitrarily long multi-line annotations for git tags View arbitrarily long multi-line annotations for git tags Feb 18, 2022
@stale stale bot added the dormant Marked by stale bot on close label Sep 28, 2022
@skyfaller
Copy link
Author

Are there obvious blockers to using draw_scrollbar as y'all once did for the revlog? #868

@stale stale bot removed the dormant Marked by stale bot on close label Sep 28, 2022
@extrawurst
Copy link
Owner

Are there obvious blockers to using draw_scrollbar

I don't think so

Repository owner deleted a comment from stale bot Jan 11, 2023
@extrawurst extrawurst added the good first issue Good for newcomers label Feb 27, 2024
@extrawurst
Copy link
Owner

yeah i think this should be pretty simple to do using the machinery for scrollbars we use in a lot of places already

@MichaelAug
Copy link
Contributor

Hi, I started working on this, i'm adding a new popup with a scrollbar for the tag annotation instead of using MsgPopup. should have a PR up soon :)

@extrawurst
Copy link
Owner

Why not the exiting one and add the scrollbar?

@MichaelAug
Copy link
Contributor

Because currently the annotation is shown using:

	fn show_annotation(&self) {
		if let Some(tag) = self.selected_tag() {
			if let Some(annotation) = &tag.annotation {
				self.queue.push(InternalEvent::ShowInfoMsg(
					annotation.clone(),
				));
			}
		}
	}

Which calls

			InternalEvent::ShowInfoMsg(msg) => {
				self.msg_popup.show_info(msg.as_str())?;
				flags
					.insert(NeedsUpdate::ALL | NeedsUpdate::COMMANDS);
			}

I didn't want to add a scrollbar to msg_popup as that is used in other places as well so decided to add a separate popup.

I'm happy to just add a scroll bar to the info popup if that's preferable

@MichaelAug
Copy link
Contributor

I want to choose the solution that's best for the project and keeps things consistent. Would you recommend adding a scrollbar to the MsgPopup, creating a new popup for tag annotations, or is there another approach you'd prefer? Thanks for your input!

@extrawurst
Copy link
Owner

you raised a good point. i had to take a look again. my first hunch was: lets use the new multiline textinput but that one does not support readonly right now and is already a rather complex beast.

So in order to keep things simple you can just add vertical scrolling to MsgPopup (using DetailsComponent maybe for inspiration as it supports multiline scrollable text view already).

Ideally we put that functionality into a separate ScrollableTextView component and reuse it between (at least) MsgPopup and DetailsComponent. Both doing this in one step or in two would be fine to me. WDYT?

@extrawurst
Copy link
Owner

@MichaelAug for more instant exchange amongst contributors feel free to join our discord: https://discord.gg/d2p2NS7m

@MichaelAug
Copy link
Contributor

MichaelAug commented Mar 5, 2024

Making a reusable component that is used in MsgPopup and DetailsComponent sounds good. I can make the change in a single PR or split into 2 (make the MsgPopup scrollable in the first PR, then refactor DetailsComponent and MsgPopup to use the generic 'ScrollableTextView' in the second PR), whichever you prefer :)

@MichaelAug
Copy link
Contributor

@MichaelAug for more instant exchange amongst contributors feel free to join our discord: https://discord.gg/d2p2NS7m

Thanks i'm already in the Discord 👍

@MichaelAug
Copy link
Contributor

I made a PR for the first part. After this is approved I'll make another PR to refactor MsgPopup and DetailsComponent to use a new ScrollableTextView component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants