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

ignore hashtag suggestions if they vary only in case #16460

Merged
merged 4 commits into from Dec 15, 2021

Conversation

weex
Copy link
Contributor

@weex weex commented Jul 3, 2021

Fixes #16449 by adding an ignoreSuggestion event.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I wonder if, instead of adding an event type, it wouldn't be better to change insertSuggestion so that it selects either the completion or token based on the case-insensitive comparison.

I also wonder if it wouldn't make sense, if the two strings differ in casing only, and one is lowercase-only, to select the other, as it is most likely more readable.

app/javascript/mastodon/reducers/compose.js Show resolved Hide resolved
@weex
Copy link
Contributor Author

weex commented Jul 5, 2021

I wonder if, instead of adding an event type, it wouldn't be better to change insertSuggestion so that it selects either the completion or token based on the case-insensitive comparison.

The issue there is that the token is already lowered when populating textAtCursorMatchesToken (https://github.com/tootsuite/mastodon/blob/main/app/javascript/mastodon/components/autosuggest_textarea.js#L11) so either we need to pass through another argument or return the non-lowered token there.

I also wonder if it wouldn't make sense, if the two strings differ in casing only, and one is lowercase-only, to select the other, as it is most likely more readable.

It might but it's probably better in another PR.

@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Jul 6, 2021

I wonder if, instead of adding an event type, it wouldn't be better to change insertSuggestion so that it selects either the completion or token based on the case-insensitive comparison.

The issue there is that the token is already lowered when populating textAtCursorMatchesToken (https://github.com/tootsuite/mastodon/blob/main/app/javascript/mastodon/components/autosuggest_textarea.js#L11) so either we need to pass through another argument or return the non-lowered token there.

Ah, I missed the token being lowercased already. It's a bit convoluted, but the initial text can easily be recovered by doing something like map.get(path).slice(position, position + token.length).

I also wonder if it wouldn't make sense, if the two strings differ in casing only, and one is lowercase-only, to select the other, as it is most likely more readable.

It might but it's probably better in another PR.

I agree, to a point, but I'd like to also avoid multiple successive behavior changes if possible.

@rkingett
Copy link

rkingett commented Jul 6, 2021

This is a welcomed proposed change.

@weex
Copy link
Contributor Author

weex commented Jul 13, 2021

Thanks @ClearlyClaire, I updated with your proposed change which cleared the inconsistency about the space.

This PR isn't perfect but it's progress so leaving this issue for now. The comments above about map.get(path).slice(position, position + token.length) might be a better way to solve if anyone else wants to tackle.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I still prefer my earlier suggestion, but this PR looks good to me and is an improvement upon the current behavior.

@weex weex closed this Aug 22, 2021
@weex weex deleted the ignore-matching-hashtags branch August 22, 2021 21:50
@ClearlyClaire
Copy link
Contributor

Was this closed by error?

@weex weex restored the ignore-matching-hashtags branch September 1, 2021 15:41
@weex
Copy link
Contributor Author

weex commented Sep 1, 2021

Yes

@weex weex reopened this Sep 1, 2021
@weex weex closed this Oct 12, 2021
@weex weex deleted the ignore-matching-hashtags branch October 12, 2021 22:47
@weex weex restored the ignore-matching-hashtags branch October 12, 2021 22:48
@FediVideos
Copy link

Is there some reason this was closed?

@weex weex reopened this Nov 23, 2021
@weex
Copy link
Contributor Author

weex commented Nov 23, 2021

Thanks for the ping. This was created when the magicstone-dev repo was my personal one and since then I've been trying to delete any branches other than main due to 2.5.1 here. So as you see from the history I've deleted and recreated the branch a couple of times, while reopening the PR is less obvious a step after that.

@weex
Copy link
Contributor Author

weex commented Dec 15, 2021

Can't tell what the problem was in CI since I suppose the logs have been deleted after a few months in DeepSource. If this gets re-run, please ping me to let me know if anything needs to be done.

@ClearlyClaire
Copy link
Contributor

It likely was an issue with DeepSource itself, and not your code (sometimes, it would basically re-check everything, not only the changed parts). I'd rebase and force-push.

@Gargron Gargron merged commit 2aafa5b into mastodon:main Dec 15, 2021
@weex weex deleted the ignore-matching-hashtags branch December 15, 2021 23:28
jesseplusplus pushed a commit to jesseplusplus/decodon that referenced this pull request Feb 10, 2022
* ignore hashtag suggestions if they vary only in case

* remove console.logs and unused args

* consistently add space when dismissing suggestions

* linting
@FediVideos
Copy link

Has this change been implemented in the latest version of Masto? At the moment I get the following results:

On Mastodon.online (v3.4.6): Try to type #HashTag but suggestions turn it into #häshtäg (with umlauts!)

On Mas.to (v3.4.6): Try to type #HashTag but suggestions turn it into #hashtag

On Mstdn.social (v3.4.6+glitch): Try to type #HashTag, successfully stays as #HashTag

@ClearlyClaire
Copy link
Contributor

Mastodon.online and Mstdn.social would have that change, not Mas.to as far as I know. The umlauts thing is interesting :D

@FediVideos
Copy link

So, it is there but broken on mastodon.online? Just tried it again and it still does that umlaut thing...

Is there a correct procedure for asking this to be fixed? (Sorry, I'm not that familiar with how github works!)

@rkingett
Copy link

Just checked on my instance, writing exchange, and it is still changing it to all lower case. I wish the opposite would happen, where all hashtags were changed to CammelCase. In order for this to work, this PR must be merged into the main branch, which it looks like it has?

@weex
Copy link
Contributor Author

weex commented Feb 24, 2022

Tried it on fosstodon (v.3.4.6_ and it seems to be working as expected. @FediVideos the best place is probably to create a new issue. Could be a subtle way in how you're using it? Timing of hitting tab vs. enter vs. space. In a new issue you could describe exactly what you're doing and mention this issue number so there's a trail for later peeps to follow.

@ClearlyClaire
Copy link
Contributor

Note that this has not landed in any release, it's only in the development version (which is currently also reported as 3.4.6).

@FediVideos
Copy link

FediVideos commented Feb 25, 2022

Weird, it is now working perfectly on mastodon.online!

I am typing HashTag, the top suggestion is hashtag but pressing enter or space leaves me with HashTag (which is exactly how it should behave!). Still have no idea what caused the umlaut thing or why the issue went away.

So, when is this expected to roll out to release versions?

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.

Hashtag suggestions are pointlessly replacing CamelCase hashtags
5 participants