Add popup for tags

[cruessler]

This closes #483.

This is a preview that has all the foundations in place so that we can now quickly iterate on the features that should be part of the first release.

@extrawurst You mentioned the ability to jump to a specific commit in the log view. Did you have something like InternalEvent::JumpToCommitInRevlog(CommitId) in mind?

The selection logic, in particular TagListComponent::move_selection, is quite similar to the one in BlameFileComponent, in particular BlameFileComponent::move_selection, so there is some potential for code re-use, but I did not yet explore that in-depth.

Let me know what you think!

[cruessler]

[extrawurst]

Really cool start! Thanks @cruessler

This is a preview that has all the foundations in place so that we can now quickly iterate on the features that should be part of the first release.

I actually think the toughest part is yet to come 🙈 I think we will want more than the current bare tag-name-list - the very least we should show with each entry: timestamp, creator (and maybe a short-desc).
Note on that: Tags can be either fat and contain their own message, author, target-commit (..) or lightweight and simply point directly to a commit.

This makes the whole process (if not already) require to be able to run async (since we need to pull commit infos on each CommitId). Albeit I am cool to make it as a sync function first (MVP style) and make the asyncification a followup.

@extrawurst You mentioned the ability to jump to a specific commit in the log view. Did you have something like InternalEvent::JumpToCommitInRevlog(CommitId) in mind?

Exactly.
Keep in mind though that tags we find in the repo are not necessarily on our current branch. We need to take this into account.

The selection logic, in particular TagListComponent::move_selection, is quite similar to the one in BlameFileComponent, in particular BlameFileComponent::move_selection, so there is some potential for code re-use, but I did not yet explore that in-depth.

That would actually be cool, there are probably even more places we have such code!

On top of what is mentioned above what is still missing:

• fetch tags (is already part of the push tags functionality)
• deleta a tag
• sort the tag list based on the timestamp

[cruessler]

Thanks for the in-depth feedback! I’ll update the PR over the next few days.

[cruessler]

@extrawurst Do you want the popup to have some sort of indicator whether a tag is annotated, e. g. bold font or a symbol?

[extrawurst]

@extrawurst Do you want the popup to have some sort of indicator whether a tag is annotated, e. g. bold font or a symbol?

I don’t see why right now. Any reason to do so? I doubt most people actually know there is a difference 🤣

[cruessler]

Fine with me, that will likely also be easier to implement. 😀 Just wanted to make sure so that I can adapt the data structures to all use cases from the start.

[cruessler]

@extrawurst Does the following layout work for you? As soon as the layout is done, I’ll refactor a bit and implement the rest.

[extrawurst]

@cruessler that looks great! lets sort the tags by the date of the tag though :)

[cruessler]

@extrawurst I’ve updated the PR and added “Delete tag” as well as sorting by timestamp.

When you said fetching tags is already part of the push tags functionality, what were you referring to? As far as I can see, the required functions are not implemented yet, but it would be relatively easy to copy and adapt the existing ones. Did you have that in mind?

[extrawurst]

@cruessler really cool!

so I just double checked and we even have a unittest test_push_pull_tags that shows that regular fetching already fetches tags aswell. so for me this is not strictly an MVP feature to have a dedicated tag-fetching. is there anything you still wanna do, otherwise please update to current master (that should fix the nightly ci steps aswell) and mark this as ready for review ❤️

edit: checking it out locally I just realize we should support actually jumping to a tag in the loglist via enter - what do you think?

[cruessler]

@extrawurst I think that’s a good idea, I’ll update the PR soon!

[cruessler]

@extrawurst I added a command “Select commit” and tested it in the gitui as well as in the linux repository. As far as I could see, jumping to a commit worked without issues.

There is one minor issue I’m not sure how to handle best. If the commit is not part of the current log because it is on a different branch or has not been loaded yet, the popup closes, but nothing else happens. That might be confusing. We could pass AsyncLog to TagListComponent to make sure the target can actually be shown before sending a command. What do you think?

[extrawurst]

@cruessler cool stuff!

[extrawurst]

the only thing that seems odd is the pageup/down behaviour, it seems one-off:

[cruessler]

You’re right! I changed that.

The behaviour came from BlameFileComponent::move_selection where it is harder to see that something is off. One might even argue that it is more correct to jump to the first invisible line because that is the first line of the next page. On the other hand, I think it makes sense to go to the last visible line because then the movement of the selection is more obvious.

[extrawurst]

ok I am going to merge this now 🥳
thanks for your support❤️