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

Introduce Device:useDPadAsActionKeys() #11900

Merged
merged 9 commits into from
May 26, 2024

Conversation

Frenzie
Copy link
Member

@Frenzie Frenzie commented May 26, 2024

  1. Non-Kindle-specific hasFiveWay behavior is changed to hasDPad and useDPadAsActionKeys. For now they remain Kindle-specific in practice, unless one sets useDPadAsActionKeys = yes in a user patch.
  2. With that disambiguation out of the way, hasFiveWay itself is further disambiguated into hasScreenKB and hasSymKey, as per the actual property being used, rather than something that tends to correlate with it. (It needn't be Kindle-specific per se, but non-Kindle devices have equivalent shortcuts with for example Shift.)
    Running the emulator with DISABLE_TOUCH=1 will set hasSymKey = yes, which can be tested with right shift.

Closes #11887.


This change is Reviewable

All uses of Device:hasFiveWay() that aren't explicitly for Kindle are changed to Device:hasDPad() and Device:useDPadAsActionKeys().

See koreader#11887.
@Frenzie Frenzie added this to the 2024.06 milestone May 26, 2024
This better reflects the actual feature it's using.
@Commodore64user
Copy link
Contributor

Technically speaking, only K4 has a ScreenKB key, so it might be wise to use something else to dub isKindle with.

@Frenzie
Copy link
Member Author

Frenzie commented May 26, 2024

useKindleKeys?

@Commodore64user
Copy link
Contributor

Commodore64user commented May 26, 2024

I think we could use hasSymKey on the other kindles with keyboards and hasScreenKB on K4.

That way hasSymKey or hasScreenKB is effectively isKindle non-touch

@Commodore64user
Copy link
Contributor

You are also ignoring #11864 which fixes the fact that content selection does not actually work atm.

@Frenzie Frenzie mentioned this pull request May 26, 2024
@Frenzie
Copy link
Member Author

Frenzie commented May 26, 2024

@Commodore64user implemented as per your suggestion

@Frenzie Frenzie merged commit 8f2bd54 into koreader:master May 26, 2024
2 checks passed
@Frenzie Frenzie deleted the 11887-reduce-fiveway-to-kindle branch May 26, 2024 19:25
self.key_events.SelectNextPageLink = { { "Shift", "LPgFwd" }, event = "SelectNextPageLink" }
self.key_events.SelectPrevPageLink = { { "Shift", "LPgBack" }, event = "SelectPrevPageLink" }
else
self.key_events.AddCurrentLocationToStack = { { "ScreenKB", "Down" } }
Copy link
Contributor

Choose a reason for hiding this comment

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

could you change this to ScreenKB and Press (instead of Down) there is something I want to try and if successful would go on KB+Down. Also, any idea why this doesn't show a notification when one uses it? normally at the top of the screen a little box appears saying, "added to stack or whatever" (yes all notifications under Screen menu are ON).

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like notification true or false is passed as an argument. DIspatcher sets arg=true.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve already tried that and still won’t show it… not sure why

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm yes, the true comes through if you add args=true but it doesn't seem to show the notification. You say it works from Dispatcher?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

the defaults

Being SOURCE_OTHER, which is described as all other sources (e.g. keyboard), which is indeed masked out unless you explicitly request notifications from all sources.

TL;DR: behaving as expected ;).

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, so how do we set a source and/or make it show the notification?

Copy link
Member

@NiLuJe NiLuJe May 27, 2024

Choose a reason for hiding this comment

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

We... don't because it's behaving as expected and keyboard shortcuts shouldn't display notifications unless the user asks for it?

Copy link
Contributor

@Commodore64user Commodore64user May 27, 2024

Choose a reason for hiding this comment

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

Yes, but this is 'different'...? I need the notification to be shown so it provides feedback on NT kindles, otherwise it would appear as if nothing was happening.

Essentially it should work same as if it was coming from dispatcher.

Copy link
Member

@NiLuJe NiLuJe May 27, 2024

Choose a reason for hiding this comment

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

Then you need a custom event that either wraps the current one between Notification:setNotifySource(Notification.SOURCE_ALWAYS_SHOW) <-> Notification:resetNotifySource() (double-check that for obvious typos) or reimplement the current one except that it passes Notification.SOURCE_ALWAYS_SHOW as the second arg to Notify.

if Device:hasScreenKB() or Device:hasSymKey() then
self.key_events.GotoSelectedPageLink = { { "Press" }, event = "GotoSelectedPageLink" }
if Device:hasKeyboard() then
self.key_events.AddCurrentLocationToStack = { { "Shift", "Down" } }
Copy link
Contributor

Choose a reason for hiding this comment

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

same here Shift + Press

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.

Resolve Device:hasFiveWay() code debt
3 participants