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

creating annotated tag broke #2126

Closed
extrawurst opened this issue Mar 13, 2024 · 10 comments · Fixed by #2139
Closed

creating annotated tag broke #2126

extrawurst opened this issue Mar 13, 2024 · 10 comments · Fixed by #2139
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@extrawurst
Copy link
Owner

this is how creating annotated tags used to work (before 0.25):

tag-annotation

now its broken. i suspect this broke during the transition to textarea in b9a2e13 by @pm100

@extrawurst extrawurst added the bug Something isn't working label Mar 13, 2024
@extrawurst extrawurst added this to the v0.25.2 milestone Mar 13, 2024
@extrawurst extrawurst added the good first issue Good for newcomers label Mar 13, 2024
@pm100
Copy link
Contributor

pm100 commented Mar 13, 2024

to be clear

  • 't' should be a single line terminated by 'return/enter'?
  • 'T' should be multi line (like commit). With Enter creating a new line and ctrl-enter being confirm and close?

@extrawurst
Copy link
Owner Author

@MichaelAug is on this one

@MichaelAug
Copy link
Contributor

The issue is with TextInputComponent handling the ctrl-a keypress to move the cursor to the beginning of line.

Input {
key: Key::Char('a'),
ctrl: true,
alt: false,
..
}
| Input { key: Key::Home, .. }
| Input {
key: Key::Left | Key::Char('b'),
ctrl: true,
alt: true,
..
} => {
ta.move_cursor(CursorMove::Head);
true

So there's multiple ways to handle this. We could make TextInputComponent not handle ctrl-a keybind since there are multiple shortcuts for moving the cursor to head. Or we could change the annotation keybind to something else. Which option would you prefer @extrawurst ?

@pm100
Copy link
Contributor

pm100 commented Mar 17, 2024

I would do what I did with all other 'known' keystrokes that map to things that are not hugely important to tuit xtarea. Filter them out at the start, you will see a set of skipped keystrokes at the start of the key handling (on the road so I can't point to the specific line)

@MichaelAug
Copy link
Contributor

Thanks @pm100, I think your filtering out code was removed in #2053 though...

@extrawurst
Copy link
Owner Author

extrawurst commented Mar 20, 2024

@MichaelAug I had a look at #2053 again and remember what happened: the cleaner way to do it is to first react to our local gitui key events and only push down whats not handled into textarea. so I prototyped the same solution for this one: #2139

please pick it up from there:

  • needs further testing
  • needs to swap out the TextInput when going to annotation mode to a multiline one

@pm100
Copy link
Contributor

pm100 commented Mar 20, 2024

@extrawurst I just saw that you removed the code that filtered out the keystrokes that have special meaning to gitui, but that are safe to not hand to tta


				// here all 'known' special keys for any textinput call are filtered out

				if key_match(e, self.key_config.keys.toggle_verify)
					|| key_match(e, self.key_config.keys.commit_amend)
					|| key_match(
						e,
						self.key_config.keys.open_commit_editor,
					) || key_match(
					e,
					self.key_config.keys.commit_history_next,
				) {
					return Ok(EventState::NotConsumed);
				}

This code in fact made sure that ctrl-a worked OK

@extrawurst
Copy link
Owner Author

@pm100 I am not sure what you mean, ctrl+a works:
Screenflick Movie 27

@pm100
Copy link
Contributor

pm100 commented Mar 20, 2024

But ctrla should NOT work. It's has a special meaning to ggitui so I filtered it out and returned as unhandled. That deleted block of code picked out all the special keystrokes that have meaning to gityi text input callers. I lacked the knowledge of who the caller was so could not conditionally filter them out. However all the filtered out strokes were for relatively obscure tta features.

I had all this explained way back in the origin nal PR ( a long time ago).

I was determined to have minimal impact, so I had no idea who the caller was. I could have filtered out in the caller, or had th caller pass a list of keys that tta should not eat. But that would have been an API change

@extrawurst
Copy link
Owner Author

extrawurst commented Mar 20, 2024

@pm100 see the above mentioned commit: the caller filters out what the caller cares about. It is working as expected. If you want to discuss this further feel free to join the discord. This derails this conversation here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants