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

Insert colons after completing nicks preceded by a list of other autocompleted nicks #822

Merged
merged 1 commit into from
Feb 4, 2018

Conversation

Manishearth
Copy link
Contributor

When pinging two people, typing nick1<tab> nick2<tab> message for autocompletion will get you nick1: nick2 message, which is kinda confusing.

We only insert the colon after the first autocompleted nick, because if a nick is used in the middle of the sentence it's mentioning the person but not targeting the message at them.

However, this breaks down when we try to ping a list of people. There should be a colon in front of each separating the names from the message. only having a colon at the end of the nick list works too, but it seems like the IRC convention is to just use multiple colons.

This patch makes it so that autocompleting a nick after a list of existing autocompleted nicks at the beginning of the message will include a colon.

@Manishearth
Copy link
Contributor Author

r? @LemonBoy

@ailin-nemui
Copy link
Contributor

nice idea, but I wonder about "should be"

in my opinion
nick1: nick2: nick3: hi does not look nice. personally in this situation I would do
nick1: <backspace><backspace>nick2 nick3: hi, in other words
nick1 nick2 nick3: hi

also this probably should take into consideration the completion_char. A popular choice is ,, in which case
nick1, nick2, nick3, hi does look more pleasant to my eyes, compared to the colon

finally you should remember that by default irssi only hilights the single-nick case (or, nick in first place). in that regard, the default behaviour is more clear with regards to the expectancy of the receiver

@Manishearth
Copy link
Contributor Author

Yeah, however the SIGNAL_FUNC API doesn't work that way because the linestart pointer is immutable and anyway a copy of the original line. This would require some major refactoring to make work.

finally you should remember that by default irssi only hilights the single-nick case

I think other clients don't. However, I could make irssi highlight any nick mention in the initial series of colon'd nicks and make this uniform, if that would be acceptable?

@Manishearth
Copy link
Contributor Author

Also, fixed the completion_char stuff. I had kept that in the back of my mind when writing this and then totally forgot.

@dequis
Copy link
Member

dequis commented Jan 20, 2018

Hm, build errors in the last commit because completion_char is a char* instead of char so you can't compare with ==.

This also highlights that completion_char can be a multi character string, so that loop needs more changes than just switching to strcmp. Unless we limit the whole thing to only commas as the completion_char...

finally you should remember that by default irssi only hilights the single-nick case

I think other clients don't. However, I could make irssi highlight any nick mention in the initial series of colon'd nicks and make this uniform, if that would be acceptable?

I think that was intended as something that keep in mind, not necessarily something to address. It's out of scope of this change anyway. Currently we have two nicks for this, hilight_nick_matches (default on) and hilight_nick_matches_everywhere (default off) and imo everyone should enable the latter (or set up equivalent hilights) but changing defaults is forbidden. Changing the behavior of hilight_nick_matches is probably not welcome either. So better not do anything there.

nice idea, but I wonder about "should be"
[snip]

Yeah, not sure this is the best change of behavior, but there's not a lot that can be done with the way completion works, like manish said. I'd love to have something like a secondary completion char and do "nick1, nick2, nick3:" with subsequent completes editing past chars, but who knows how to get to that in a sane way. On the other hand nick1: nick2: nick3: is way better looking than nick1: nick2 nick3, so uh.

@Manishearth
Copy link
Contributor Author

Hmm. For string valued completion_chars it seems like there will be a lot if edge cases, might just restrict this to single-char values

…completed nicks

When pinging two people, typing `nick1<tab> nick2<tab> message` for
autocompletion will get you `nick1: nick2 message`, which is kinda
confusing.

We only insert the colon after the first autocompleted nick,
because if a nick is used in the middle of the sentence it's mentioning
the person but not targeting the message at them.

However, this breaks down when we try to ping a list of people. There
should be a colon in front of each separating the names from the
message. only having a colon at the end of the nick list works too,
but it seems like the IRC convention is to just use multiple colons.

This patch makes it so that autocompleting a nick after a list of
existing autocompleted nicks at the beginning of the message
will include a colon.
@Manishearth
Copy link
Contributor Author

Done. I can attempt to handle non-char completion_chars, but that seems like an edge case that itself is fraught with edge cases, so we just bail early then (except in the "at beginning of line" case to preserve previous behavior)

@ailin-nemui
Copy link
Contributor

looks ok to me, @dequis ?

@ailin-nemui ailin-nemui added needs testing auto-merge This PR is scheduled for merge if no further comments are opened labels Jan 24, 2018
@ailin-nemui
Copy link
Contributor

@irssi/developers

@ailin-nemui ailin-nemui merged commit c25e122 into irssi:master Feb 4, 2018
@ailin-nemui ailin-nemui added this to the 1.2.0 milestone Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge This PR is scheduled for merge if no further comments are opened needs testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants