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

Enable Highlight action on single word selection #6114

Merged
merged 3 commits into from May 5, 2020

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented May 3, 2020

Also add "Fulltext search" as an option for highlight action.
See #6111 (comment). Closes #6111.

@protist, in this highlight_search_singleword.zip are the 2 modified files, if you want to confirm this works for you.

image


This change is Reviewable

Also add "Fulltext search" as an option for highlight action.
@poire-z
Copy link
Contributor Author

poire-z commented May 3, 2020

Dunno if we should also add Dictionary lookup.
It might work well only on single word selection - although I dunno if dicts can/do have multiwords word definitions and if sdcv works with that well.
But for a user that checks "Enable on single word selection", it can be handy I guess.

@Frenzie
Copy link
Member

Frenzie commented May 3, 2020

@poire-z Works fine afaict ;-)

Screenshot_20200503_212113

@mergen3107
Copy link
Contributor

@poire-z
Isn’t single word dictionary lookup currently set as default? On 2020.04.02 release at least. That’s what I extensively use right now

@poire-z
Copy link
Contributor Author

poire-z commented May 3, 2020

Isn’t single word dictionary lookup currently set as default?

Yes it is. But people enabling the new option "Enable (highlight action) on single word selection" to be able to use one of the highlight action wouldn't have it in the set. So my question about including it.

These highlight actions can also be cycled with a gesture, so I guess some people might stick with it, and would cycle depending on the book content, and the need for quick "translate" or "wikipedia", or the good usual of "dictionary".

@poire-z
Copy link
Contributor Author

poire-z commented May 3, 2020

OK, this is buggy: when enabling "on single word", selecting multiple words only uses the first :)

@poire-z
Copy link
Contributor Author

poire-z commented May 3, 2020

OK, added "Dictionary", fixed bug.
Should we change the order, in the menu and in the cycling?
I did not, to not mess with current users that may be used to the current order.
image

@NiLuJe
Copy link
Member

NiLuJe commented May 3, 2020

Should we change the order, in the menu and in the cycling?

Current order looks fine to me ;).

@protist
Copy link

protist commented May 4, 2020

Thank you so much for the quick work! This seems to work well for me. I had a few comments regarding the implementation though, back in #6111.

@poire-z
Copy link
Contributor Author

poire-z commented May 4, 2020

#6111 (comment)

What short hold long hold at end of selection (3s)
Single word lookup dict button menu
Single word lookup with [x] Enable on single word lookup highlight action button menu
Multiple words selection highlight action button menu

@protist: here's the 3rd version: highlight_search_singleword3.zip

@protist
Copy link

protist commented May 5, 2020

@poire-z Looks great! This seems to work really well for my device! Thank you!

The last thing is I find the UX quite confusing for the settings. In particular, the Enable on single word lookup phrasing, but also the implied actions for the other inputs. I wonder if it would be possible to configure it similar to the gesture manager. That is, have a first tier where you specify the input trigger, i.e. select one of

  • Single word lookup — short hold
  • Single word lookup — long hold
  • Multiple words selection — short hold
  • Multiple words selection — long hold

Then a second tier where you specify one of the six associated actions, i.e. the popup dialog, highlight, translate, Wikipedia, dictionary, fulltext search.

@poire-z
Copy link
Contributor Author

poire-z commented May 5, 2020

Thanks for the feedback.

I wonder if it would be possible to configure it similar to the gesture manager

Could make sense. But as it may have less simple implications to other code, I let that for some (someone else's :) other PR.

@protist
Copy link

protist commented May 5, 2020

Thanks again @poire-z. That definitely sounds reasonable.

(Just for the record, ideally I think the multiple word selection would be different. e.g. if single-word lookup, short hold, is search, it doesn't make as much sense to force multiple-word selection, short hold, to be search too, but perhaps something like dictionary instead. Hence the finer tuning would be useful.)

@poire-z
Copy link
Contributor Author

poire-z commented May 5, 2020

(Just for the record too :)
I think the contrary: there's no real reason to have single word vs multiple words work differently - moreover with search: what you want to search for (or wikipedia lookup) might be one word, or two words: no reason for the user to wonder and think about what will happening.
It only makes sense with dictionary lookup, where we assume(d) dictionary have only definitions for single words.

@protist
Copy link

protist commented May 5, 2020

Ooops, sorry, I mistyped. In my usage, probably 90% of the time I select multiple words it's for a highlight. For me, single words are never highlights, and are either search or dictionary.

@poire-z poire-z merged commit 429f4bf into koreader:master May 5, 2020
@poire-z poire-z deleted the single_word_highlight_action branch May 5, 2020 16:56
@Frenzie Frenzie added this to the 2020.05 milestone May 5, 2020
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.

Expose search icons immediately on the long-press menu
5 participants