autokey icon indicating copy to clipboard operation
autokey copied to clipboard

Selecting Match phrase case to typed abbreviation also selects its opposite, Ignore case of typed abbreviation

Open josephj11 opened this issue 3 years ago • 30 comments

Classification:

Bug

Reproducibility:

Always

AutoKey version: 0.95.10

Both

If the problem is known to be present in more than one version, please list all of those.

Installed via: debs from GitHub

Linux Distribution: kubuntu 18.04 and others

Summary

Selecting Match phrase case to typed abbreviation also selects its opposite, Ignore case of typed abbreviation in the GUI

Steps to Reproduce (if applicable)

Define a phrase and select Match phrase case to typed abbreviation

Expected Results

Just that one option should be selected

Actual Results

Ignore case of typed abbreviation also becomes automatically selected - which should be mutually exclusive with the selected option

Notes

In 0.95.10, if you define a phrase and select Match phrase case to typed abbreviation, it automatically also selects Ignore case of typed abbreviation which makes no sense to me. This only works one way. Selecting Ignore case of typed abbreviation does not auto-select Match phrase case to typed abbreviation. I recreated this on both front ends.

I find it most curious that this bug appears in both front ends. I thought most of that code was disjoint.

I did not check which option actually takes effect, but I believe it honors the first option. If it didn't, we would probably have seen numerous error reports starting shortly after 0.95.10 was released (assuming that's where the bug was introduced - which has not been investigated.)

josephj11 avatar Jul 29 '21 08:07 josephj11

Are we sure this is a bug? This seems like intended (if confusing) behavior;

Selecting Ignore case of typed abbreviation means all of the following will launch the abbreviation;

  • "trigger" = lower case
  • "TRigger" = mixed case
  • "TRIGGER" = upper case

Selecting "Match phrase case to typed abbreviation" by default will allow all of the above to fire;

  • lower case will put the abbreviation in all lower case
  • mixed case puts it exactly as it is in the phrase
  • all caps will put the phrase in all caps.

If you think about what they mean and not what they say it is clear why both have to be selected for match case. Without ignore case, match phrase wouldn't do anything special unless it is allowed to ignore the input abbreviation case.

I'd agree that this is unclear and unintuitive, and as far as I know not documented anywhere.

Perhaps change the phrasing of ignore from negative to positive? Like "trigger abbreviation regardless of case used"

sebastiansam55 avatar Aug 25 '21 11:08 sebastiansam55

I'm not sure I understand this, but maybe the suggested relabeling of the option clears it up. As long as it really reflects what the code is doing.

josephj11 avatar Aug 26 '21 21:08 josephj11

Would these be more clear?

  • Match phrase case (ignore typed abbreviation case)
  • Match typed abbreviation case (ignore phrase case)

Elliria avatar Oct 30 '21 02:10 Elliria

@Elliria Thanks for getting back to this. I had forgotten about it. No.

This whole bit of UX is squirrelly. I know what's going on and I'm still staring at it after longer than I care to admit. :)

I think I may have solved this Chinese puzzle box.

Try this:

Modify phrase to match capitalization of abbreviation Abbreviation is case insensitive

If the first option (note that they are reordered) is selected, the second option should be disabled because it has to be true or the first option becomes moot. It's no longer an option. It could still be automatically marked as selected, but being grayed out makes it clear that it cannot be changed. (Is this easy enough to implement?)

Contrary to what I believe is the current implementation, the abbreviation can be case insensitive without forcing the phrase to match it. All abbreviation variations would trigger, but the phrase would be unmodified.

This would add functionality to AutoKey.

Having (hopefully) solved one problem, for the moment, I will leave the issue of what this does to backward compatibility for someone else to look at. :)

My guess is that it will have no effect on older phrases, but that needs to be verified.

This might no longer be a good first issue (see tags), but I'll leave that decision to the devs.


If someone wants to get really creative, add an option for the abbreviation to be a regex. Then the phrase could be one too with things like group references, etc. But, that sounds like a serious piece of work. And, it's probably more power than anyone actually needs. It's just cool as a concept - like having sed built into AutoKey.

If someone is crazy enough to try this, it would need a separate enhancement issue.

josephj11 avatar Oct 30 '21 19:10 josephj11

From where I sit, the first option is about the abbreviation and the second option is about the output. That isn't being made clear to the user. The problem is the unfortunately awkward, inconsistent, and uninformative wording of these two options:

  • Ignore case of typed abbreviation
  • Match phrase case to typed abbreviation

How about we replace them with two options that are simplified and let the user know the second option also requires the first option? Below are 9 possibilities, with my favorite being example 4:

Example 1:

  • Abbreviation is case-insensitive.
  • Abbreviation case determines output case (must also be case-insensitive).

Example 2:

  • Case-insensitive abbreviation.
  • Case matches typed abbreviation (must also use case-insensitive abbreviation).

Example 3:

  • Accept any abbreviation case.
  • Match typed abbreviation case (must also accept any abbreviation case).

Example 4:

  • Accept any abbreviation case.
  • Match abbreviation case (must also accept any abbreviation case).

Example 5:

  • Accept any abbreviation case.
  • Output case matches abbreviation case (must also accept any abbreviation case).

Example 6:

  • Ignore typed abbreviation case.
  • Match typed abbreviation case (also requires "Ignore typed abbreviation case").

Example 7:

  • Ignore typed abbreviation case.
  • Match typed abbreviation case (also ignores case).

Example 8:

  • Ignore typed abbreviation case.
  • Match typed abbreviation case (must also ignore case).

Example 9:

  • Ignore abbreviation case as typed.
  • Match abbreviation case as typed (must also ignore typed case).

Elliria avatar Oct 31 '21 03:10 Elliria

How about this? It's the same as my original proposal, including the other suggested changes, but uses your improved wording for the first option.

Abbreviation case determines output case Abbreviation is case insensitive

Any thoughts on the changes to functionality that were proposed?

josephj11 avatar Oct 31 '21 07:10 josephj11

The order of the two options probably doesn't matter, but it wouldn't hurt to switch those around.

The use of "case-insensitive" is better than "ignore case" for the one option, because "ignore" is probably the biggest stumbling-block in the existing option.

I'd do a slight bit of tweaking to your two options (I've put them in the original order, but they could be reversed):

  • Abbreviation is case-insensitive
  • Output case is determined by abbreviation case

You're right that the case-insensitive one can already be used by itself to let you type the abbreviation in any case you like without affecting the case of the output. That option isn't about the phrase at all and is just about the input (the abbreviation), which is as it should be. The other one, however, is just about the output, but requires freedom of input to pull off its feat.

I don't think the case-insensitive option should be disabled when the match-case option is selected, because that would suggest that the case-insensitive option is not available at all when it's actually in full use while the match-case option is enabled. I understand why it might be helpful as far as indicating to the user that they now cannot change that setting unless some other setting is changed, but it doesn't tell them anything, which I think is the current problem. The two options are fine as they are and they both work nicely, but their wording leaves the user in a state of not understanding what's happening, which was why this issue was opened to begin with.

As for the regex idea, I suppose anything that offers the user more freedom or power could be a good thing, but I'm not sure how hard something like that would be to code. It's probably a monster.

Elliria avatar Oct 31 '21 17:10 Elliria

I like your

Output case is determined by abbreviation case

better.

The rest is mainly cosmetic and will be fine either your way or mine.

I did suggest marking the second option as selected when disabled. That ought to give the user some idea of what's going on. If it's still confusing, a longer tooltip could be added when hovering over one or both of the options.

Again, either approach is a big improvement and would be fine with me.

josephj11 avatar Nov 01 '21 08:11 josephj11

Yeah, the cosmetic stuff isn't important and there's nothing actually wrong with the options. I think what worries me about disabling the other option without some sort of obvious or blatant explanation (like this comment in brackets or the wording of the options) is that it may leave the user wondering why the other option has been enabled and can't be disabled, and as a result, wondering if something is wrong. I really like your idea of the tooltip. That would help a lot.

Elliria avatar Nov 01 '21 13:11 Elliria

It may be that I'm going about things the wrong way, but I think I do actually have a use case for enabling Match phrase case to typed abbreviation while disabling Ignore case of typed abbreviation. I have an abbreviation for e.g. the word "because", which is "bc". But I live in British Columbia, so I type "BC" a lot, which I don't want expanded to "BECAUSE". What I wanted to do was have two separate abbreviations for the phrase "because", "bc" and "Bc", both of which Match phrase case to typed abbreviation but do not Ignore case of typed abbreviation. This way I only need to configure the phrase once - the only other solution I can see is to duplicate the configuration between two separate phrases, "because" and "Because".

The UI does not allow me to do what I want, which is how I ended up here. FWIW, this does seem to work fine when I edit the JSON for the phrases directly, and I don't see anything in the code that suggests that it shouldn't. My proposal is to completely remove the connection in the UI between these two options, while making some label changes as described above to make the whole thing clearer.

Sorry to bump an old issue - I'm happy to submit a PR myself if these ideas make sense.

wohanley avatar Apr 24 '23 17:04 wohanley

@wohanley Welcome to the AutoKey community!

We're always happy to have new contributions. If an issue is still open, it's not too "old". (And that might also apply to a closed issue, if there's something new to consider.)

This particular problem is a bit convoluted, so I'll have to come back to it when I have a little more time to think about it. (hopefully, very soon) We'll see if others comment as well.

josephj11 avatar Apr 24 '23 20:04 josephj11

That's a valid case, @wohanley, but it's tricky. I'd say that your idea of duplicating the phrase to address the two different use cases is a good one. As an alternative, if you're using AutoKey 0.96.0, you could use a script to handle that abbreviation and put the engine.get_triggered_abbreviation() API call into a conditional statement that checks its case and parses it differently based on the result. I can't test it, because I use AutoKey 0.95.10, but it should work.

Elliria avatar Apr 24 '23 21:04 Elliria

The question for me is: What downsides, if any, would there be if we modify AutoKey as the OP suggests?

Off hand, since the user can still manually select the related option, there doesn't seem to be any reason not to do this.

It would be nice to know why it was done this way to start with, but the programmer who implemented it is probably gone from our project. (Does git blame show anything useful?)

@sebastiansam55 @BlueDrink9 @Elliria : any objections? We can always put it in develop or in a feature branch and see what happens when it gets used.

josephj11 avatar Apr 26 '23 22:04 josephj11

My suggestions above for renaming the two options to make their behavior more clear make it obvious that the link between the two is necessary. If you unlink them, then I'm not clear on how AutoKey would then determine what you want, because the alternative is that it matches the typed abbreviation.

I suppose the only way to find out is if the OP submits a PR and we can test it to see how it works. I'm good at breaking things, so if there's a way to get it to fall over, I should be able to find it. If not, then great.

My concern, though, is that although this is a valid use-case for something other than the currently-available behavior, it's a niche case that's probably going to be more of an exception than a rule. It would be nice to have it as an option for anyone who wants it, though, and since @wohanley most likely isn't our only BC user, this feature would probably be appreciated by others.

Either way, it can definitely be scripted with a conditional statement. And if @wohanley forks the AutoKey project and submits the PR from his fork, we can test it from there. The Trying out a clone of AutoKey steps in the Running Tests page of the wiki will work regardless of which GitHub repository the code is in and don't require installation.

Elliria avatar Apr 28 '23 04:04 Elliria

This discussion inspired me to create a workaround script. It's only pseudocode so far. If it proves interesting, I'll post about it, etc...

Got as far as rough draft code that needs work.

josephj11 avatar Apr 28 '23 21:04 josephj11

Thanks, both of you, for the discussion, although I think I'm not familiar enough with AutoKey to grasp it all. I've got a PR that I'll submit in a minute, because I think that might be the clearest way to demonstrate what I'm talking about. Basically, my situation is that I type acronyms interspersed in ordinarily-cased text far more often than I type full words in all caps (I'd expect this is true for most people?). So, what would be best for me is:

  • I type "bc": it's replaced with "because"
  • I type "Bc": it's replaced with "Because"
  • I type "BC": autokey does nothing

The list of acronyms I use is open-ended, so this is how I'd like text replacement to work in general. That is, there's nothing special about "bc".

The PR I'll open lets me achieve the above, but it sounds like we're anticipating some problems with it that aren't obvious to me. So, if you want to kick it around, thanks!

wohanley avatar May 09 '23 00:05 wohanley

It's an interesting idea and will solve a specific issue, but it would need to be an optional addition to AutoKey rather than a replacement for what's already there.

The way AutoKey currently handles abbreviations when it's told to Match phrase case to typed abbreviation is to do this:

  • bc is deleted and replaced with because
  • Bc is deleted and replaced with Because
  • BC is deleted and replaced with BECAUSE

With your proposed change, it would do this:

  • bc is deleted and replaced with because
  • Bc is deleted and replaced with Because
  • BC is deleted

This is because it's important to enable Removed typed abbreviation in the abbreviation's settings so that you don't end up with bcbecause or BcBecause or BCBECAUSE. As a result, your final bullet-point that does nothing would, instead of doing nothing, have to type BC to replace the removed abbreviation with what you'd like to see there.

When you do this with a phrase, AutoKey doesn't change the contents of the phrase in any way other than by changing its case at your request. It won't type something different or leave the contents out. Something like that would have to be handled by a script, and should be pretty easy to code (we can help if you run into any walls, because we've encountered tons of walls, too).

Either way, as you can imagine, either one of these two different behaviors would be a problem for the person who wishes the other outcome. As a result, I think they really should be two separate options and/or the new one should be a script rather than a modification to the abbreviation process, but I think it would make a nice addition to AutoKey for those who need it.

Elliria avatar May 13 '23 17:05 Elliria

Ah, sorry, I should stress that I'm still using two separate abbreviations, "bc" and "Bc", with neither ignoring the typed case - that is, typing "BC" just doesn't trigger AutoKey at all. For this to work with a single abbreviation, I agree it gets a bit problematic. But I don't feel the need to address that - from my perspective it's fine to have two slightly-duplicative abbreviations, I just need the UI to allow me to set them up. To be totally clear, I don't want to change AutoKey's triggering or text insertion behaviour at all.

wohanley avatar May 13 '23 18:05 wohanley

Ah, okay. That sounds like a reasonable way to do it and is a lot quicker and simpler than the scripting would have been. Is it working solidly for you now?

Elliria avatar May 13 '23 20:05 Elliria

Yep, the version in my fork has been working for me.

wohanley avatar May 13 '23 23:05 wohanley

That's great news. Is there a way that that will also work for those of us who want because and Because and BECAUSE? Or can it be put into place alongside what we currently have that already gives us those?

Elliria avatar May 14 '23 01:05 Elliria

Yes, just like before, you can configure an abbreviation to trigger on any input case and match it in the output, you just don't have to.

wohanley avatar May 14 '23 16:05 wohanley

Oh, that sounds perfect. What do you think, @josephj11 and @sebastiansam55? I'm all for options as a great way to make everyone happy.

Elliria avatar May 14 '23 20:05 Elliria

This is the sort of logic that confuses me. I have to think about it. We would need at least one case where it would do something wrong or unexpected to reject it. I don't know if such a case exists. @Elliria if you can't break it then it should get merged into develop.

josephj11 avatar May 14 '23 23:05 josephj11

I'll do my best to break it, @josephj11, if you do a pull request @wohanley, and if I can't, it will make a nice addition to AutoKey.

Elliria avatar May 16 '23 02:05 Elliria

OK, I updated my PR at #856. Let me know if anything's broken for you.

wohanley avatar May 16 '23 20:05 wohanley

@wohanley Got a PR notification, but GitHub couldn't find it when I clicked the link in the email. It did show me the previous changes, but those don't have the option to merge.

All three individual commits are there, but they all say: This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Please see if you can figure out what happened.

josephj11 avatar May 17 '23 06:05 josephj11

@josephj11 I'm not sure how the links would be constructed in a notification email, but if you go directly to https://github.com/autokey/autokey/pull/856 I think it should be OK (at least it is for me). What probably happened is that the email arrived before a force push I did to my fork and the links are no longer valid.

wohanley avatar May 17 '23 16:05 wohanley

@wohanley Maybe that's what happened. It looks OK now.

I have asked our devs to review it before it gets merged. IDK enough to evaluate it, but what I do understand looks reasonable.

josephj11 avatar May 17 '23 17:05 josephj11

As you probably both already know, I'm loving what it does.

Elliria avatar May 21 '23 18:05 Elliria