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

Screenshots on Non-Touch #11802

Merged
merged 3 commits into from
May 20, 2024
Merged

Screenshots on Non-Touch #11802

merged 3 commits into from
May 20, 2024

Conversation

Commodore64user
Copy link
Contributor

@Commodore64user Commodore64user commented May 12, 2024

PR adds ability to capture screenshots on devices with keyboards and other non-touch kindles.

I've had to turn ScreenKB into a mod key, hoping that is not an illegal move..?

on devices (mostly kindles) with keyboards: Alt + Shift + G
on kindle 4: ScreenKB + Menu


This change is Reviewable

@Frenzie
Copy link
Member

Frenzie commented May 14, 2024

I've had to turn ScreenKB into a mod key, hoping that is not an illegal move..?

It kind of sounds like it could just take screenshots? What's the reason for making it a modifier?

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 14, 2024

I've had to turn ScreenKB into a mod key, hoping that is not an illegal move..?

It kind of sounds like it could just take screenshots? What's the reason for making it a modifier?

Two reasons really, number one related to screenshots, i could not get it to work with multi-key presses without becoming one and number two, with #11749 it should be integrated and utilised in a much more efficient way.

I didn't want to add this to that PR as that is getting a bit too large and it is mainly aimed at improving DPad

@Frenzie
Copy link
Member

Frenzie commented May 14, 2024

i could not get it to work with multi-key presses without becoming one and number two

What does that mean?

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 14, 2024

i could not get it to work with multi-key presses without becoming one and number two

What does that mean?

It means it would not accept triggering screenshot by pressing two keys at the same time, some cries for help are found from here #11295 (comment) onwards

@Frenzie
Copy link
Member

Frenzie commented May 14, 2024

But what does a dedicated key have to do with pressing two keys?

@Commodore64user
Copy link
Contributor Author

But what does a dedicated key have to do with pressing two keys?

ScreenKG is not used at all currently, if that is what you are referring to

@Frenzie
Copy link
Member

Frenzie commented May 14, 2024

Right, so shouldn't it just take screenshots?

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 14, 2024

Right, so shouldn't it just take screenshots?

No, it would be to easy to accidentally (and unnecessarily) trigger, also not worth a button just for that, more importantly, having it as a mod is more valuable as it opens up a door to more shortcuts see #11749.

@Frenzie
Copy link
Member

Frenzie commented May 14, 2024

Pinging @NiLuJe

@NiLuJe
Copy link
Member

NiLuJe commented May 14, 2024

Fairly ambivalent about all this, so, sure, why not ;).

For context about the "easy to accidentally press" comment, the K4 features a single row of buttons at the bottom, the layout being [Back] [KB] [[DPAD]] [Menu] [Home], so, sure that makes sense ;).

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 14, 2024

@Frenzie I'll add the useDPadwhatever here later otherwise we'll have a conflict later on.
Edit: wait no, I don't think so. Never mind.
Edit2: yeah, not using the same conditions so, never mind.
Edit3: or review it and let me know.

@Frenzie
Copy link
Member

Frenzie commented May 14, 2024

I suspect in its current form it'll crash?

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 14, 2024

I suspect in its current form it'll crash?

not at all, I just meant because I am using elseif Device:hasKeys() and Device:hasPageUpDownKeys() but then I realised it is not the same case as hasDPad and hasPageUpDownKeys. Also, it is nested inside noTouch.

should be fine as is I believe. unless you think it needs amending.

@Frenzie
Copy link
Member

Frenzie commented May 14, 2024

But hasPageUpDownKeys is not a function and should therefore crash, is what I meant.

@Commodore64user
Copy link
Contributor Author

you are right, it isn't yet, yes it should crash.

Perhaps we should finish the other and merge both at the same time? what do you reckon?

@Frenzie
Copy link
Member

Frenzie commented May 14, 2024

That's fine.

@Frenzie Frenzie added this to the 2024.06 milestone May 20, 2024
@Frenzie Frenzie added the NT Non Touch devices label May 20, 2024
@Frenzie Frenzie merged commit 6c7e2a9 into koreader:master May 20, 2024
3 checks passed
@Commodore64user Commodore64user deleted the screenshots branch May 20, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NT Non Touch devices
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants