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

Allowing Space to insert words, cleanup on setSuggestions, remove using isThereText #347

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

alexknop
Copy link
Contributor

Problems:

  1. A user cannot type a partial word, see the suggestion as the first one, and press 0(Space) to insert it. It would only insert what they've partially typed. I do think it should act like OK and insert.
  2. setSuggestions() had two functions, where 1 was just to call the other and include a needed parameter. I've cleaned up the duplicate code and am always including the parameter.
  3. isThereText() was used alongside getTextBeforeCursor() which was rather redundant, since both check the TextField to see if there is existing text. isThereText is only used for one if statement in determineNextWordTextCase() and I changed it to instead utilize getTextBeforeCursor() so that isThereText can be unused.

@sspanak
Copy link
Owner

sspanak commented Aug 14, 2023

A user cannot type a partial word, see the suggestion as the first one, and press 0(Space) to insert it. It would only insert what they've partially typed. I do think it should act like OK and insert.

This is not a bug, it's a feature 😄. If you want the entire word, press OK, if you want to cut it, press SPACE. I really don't get what's the problem here. On the other hand, with your changes, there is no option to type a part of a word and the OK action is duplicated.

Either way, I will not accept this particular change.

  1. and 3. are fine, provided that they are tested and they work.

@alexknop
Copy link
Contributor Author

Why would a user type part of a word?

@sspanak
Copy link
Owner

sspanak commented Aug 15, 2023

Well, users should be able to type any word, not only the ones in the dictionary, right?

For example, in my language, the words, especially the verbs, have the same root, but different suffixes depending on gender, number, tense and whatnot. Sometimes, it turns out the word I mean to type is missing, but a similar that starts with the same letters exists. In such cases I hit SPACE and cut the longer word.

In another scenario, I may want to cut a word and type some different ending or combine it with another shorter word. I do this occasionally, as well.

And I already realized it is simply impossible to add all words in all languages, so there will always be someone in my situation, even if they speak another language.

By the way, I didn't invent this idea, Nokia did some 20 years ago and it must have been for a reason. I find it useful and probably many people before me did, so I would like to keep it.

@alexknop
Copy link
Contributor Author

Makes sense, not all words are in there. I am going to remove the space to insert part and keep the rest because I believe that is worth looking into.

@alexknop alexknop force-pushed the cleanup branch 2 times, most recently from 9454be5 to e8ffd8e Compare August 17, 2023 18:35
@alexknop
Copy link
Contributor Author

Okay, the Space key code is changed back, but please look at the other things which are just code cleanup

@alexknop alexknop force-pushed the cleanup branch 2 times, most recently from 3d09806 to b3d04c6 Compare August 23, 2023 14:35
@sspanak
Copy link
Owner

sspanak commented Aug 28, 2023

Okay, the Space key code is changed back, but please look at the other things which are just code cleanup

Done and this time as was paying more attention. 🙂

Taking away isThereText indeed looks valuable. I am willing to test it a bit and if there are no issues within 24 hours, I will accept it. It's what I always do, even with my code.

Now, removing the setSuggestions() with argument - I don't aggree with that. Yes, from technical perspective it serves no purpose, but from human perspective it provides convenience.

As I was writing the code, I found that it is annoying and sometimes unclear to use setSuggestions(null, 0). "What does null, 0 mean? I just want to clear the suggestions, why do I have to think what's the proper value for the second argument?" As I am thinking of something completely different and I just want to clear the suggestion list, I don't want to stop and ask myself such questions. And there are many other convenience functions in the code, not just this one. I've decided, if I am writing most of the code, I should feel most comfortable with it.

So let's keep both overloads of setSuggestion(), please.

@alexknop
Copy link
Contributor Author

sounds good, i'll change back setSuggestions.
Thank you for the review!

@sspanak
Copy link
Owner

sspanak commented Aug 29, 2023

It is good from my side too. I am currently stabilizing version 26.0, so no new changes are accepted, unless they fix something in #358. I'll test and merge this after that.

@sspanak sspanak mentioned this pull request Dec 14, 2023
@sspanak sspanak changed the base branch from master to summer-bugs December 15, 2023 15:18
@sspanak sspanak merged commit bb9d276 into sspanak:summer-bugs Dec 15, 2023
1 check passed
sspanak pushed a commit that referenced this pull request Dec 21, 2023
Co-authored-by: Alex Knop <alexknoptech@protonmail.com>
sspanak added a commit that referenced this pull request Dec 21, 2023
* removed isActive and fixed a startup crash in onEvaluateInputViewShown() (#364, #385, #389)

* fixed the MainView is visible in dropdowns or other non-text inputs (#367)

* fixed logging settings not being applied at startup sometimes

* code cleanup: remove using isThereText (#347)

* removed the Messenger Lite hacks, since the app has been discontinued and no longer works

---------

Co-authored-by: alexknop <knopalex@msu.edu>
Co-authored-by: Alex Knop <alexknoptech@protonmail.com>
@GiorgosXou
Copy link

The "space-input" feature is a really usefull and common one among old t9 layouts. It would be nice if there was an option for it

@sspanak
Copy link
Owner

sspanak commented May 12, 2024

@GiorgosXou, merged pull requests are not the right place for making requests. Please, create a new issue whenever you want to do so.

To answer your question, accepting words and automatically adding space after them is possible with the OK key.

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.

3 participants