search

pgaskin / NickelMenu

The easiest way to launch scripts, change settings, and run actions on Kobo e-readers.
https://pgaskin.net/NickelMenu
MIT License
556 stars 28 forks source link

Implement nickel_orientation action #70

Closed pgaskin closed 4 years ago

pgaskin commented 4 years ago

closes #67

pgaskin commented 4 years ago

So far, this works fine with ForceAllowLandscape enabled on devices without an orientation sensor. @stiivgiaco, you can test this now if you want (KoboRoot.tgz).

menu_item:main:Portrait:nickel_orientation:portrait
menu_item:main:Landscape:nickel_orientation:landscape
menu_item:main:InvPortrait:nickel_orientation:inverted_portrait
menu_item:main:InvLandscape:nickel_orientation:inverted_landscape
menu_item:reader:Portrait:nickel_orientation:portrait
menu_item:reader:Landscape:nickel_orientation:landscape
menu_item:reader:InvPortrait:nickel_orientation:inverted_portrait
menu_item:reader:InvLandscape:nickel_orientation:inverted_landscape
stiivgiaco commented 4 years ago

installing as we speak to my kobo clara 👍 thanks in advance @pgaskin

stiivgiaco commented 4 years ago

@pgaskin thanks so much for this! I have installed and tested it on my Kobo Clara HD.

All orientation options work however if a user wants to go from portrait to landscape, he/she will have to switch the orientation through the ForceAllowLandscape menu first. Meaning trying to switch from Portrait (using menu_item:main:Portrait:nickel_orientation:portrait) to Landscape (using menu_item:main:Landscape:nickel_orientation:landscape) doesn't switch the orientation.

pgaskin commented 4 years ago

I have installed and tested it on my Kobo Clara HD.

Thanks for testing it!

to switch the orientation through the ForceAllowLandscape menu first

That's a known issue (see the comments in the code review above). I'll probably be adding some messages to help users who don't realize that.

stiivgiaco commented 4 years ago

Hadnt realised that there was the sleep functionality also implemented. Just added that to the config file and also works flawlessly. You rock!

NiLuJe commented 4 years ago

Without ForceAllowLandscape:

With ForceAllowLandscape:

NiLuJe commented 4 years ago

So, short of the weird "not quite sticky across views" without the setting, that works pretty much as intended, I guess?

pgaskin commented 4 years ago

if you rotate the device, that'll only take effect if you swap view (i.e., Home <-> Library).

Can you clarify what you mean by this?

Which means a swap from Portrait-lock enables the Landscape lock. Again, gyro is useless in the current view (In the Reader, breaking the lock by switching to auto-rota re-enables the gyro. But before that, you couldn't even invert via gyro).

So that's working as intended (I eventually decided to lock it and disable invert). If you think it's a good idea, I can allow gyro inversion to work for portrait/landscape (or have an option which allows it).

(Icon is updated, FWIW).

Yes, that makes sense, since we don't actually disable the sensor (it will still attempt to change stuff and update the icon), just prevent it's readings from having any effect by limiting the mask to our new rotation.

NiLuJe commented 4 years ago

if you rotate the device, that'll only take effect if you swap view (i.e., Home <-> Library).

Which means a swap from Portrait-lock enables the Landscape lock. Again, gyro is useless in the current view (In the Reader, breaking the lock by switching to auto-rota re-enables the gyro. But before that, you couldn't even invert via gyro).

So that's working as intended (I eventually decided to lock it and disable invert). If you think it's a good idea, I can allow gyro inversion to work for portrait/landscape (or have an option which allows it).

(Icon is updated, FWIW).

Yes, that makes sense, since we don't actually disable the sensor (it will still attempt to change stuff and update the icon), just prevent it's readings from having any effect by limiting the mask to our new rotation.

Since the icon matches the usual "manual" lock state, it vaguely makes sense to me that the behavior should match too ;).

That said, I'm absolutely not the guy to ask about actual experience with this: I pretty much do everything in Portrait ^^.

pgaskin commented 4 years ago

Unrelated, but the Rota menu is "sticky" on the Homescreen: you have to select an option to dismiss it, tapping outside doesn't do so.

I've noticed that before... it's quite an unusual menu (remember the sizing issues we had a while back with the rotation patch?).

This time, the rotation sticks permanently. I have to re-tap "auto-rotate" to actually get the gyro working. (And the icon isn't updated, i.e., that was the first test: auto -> InvP, so icon is till set to auto).

Yes, this makes sense, as it's Nickel which enforces the restrictions for views (we set it either way, so it just only takes effect when the current view allows it). Regarding the gyro being disabled completely, see my above comment ("If you think it's a good idea, I can allow gyro inversion to work for portrait/landscape (or have an option which allows it)").

Same behavior as previously with the Swap. Lock is enabled, gyro is down for the count, even across views (re-tapping the current lock restores the gyro, much like tapping auto-rota).

That's good, it's what I intended.

From the Home-screen, a swap from Portrait lock does NOT enable the Landscape lock, but the gyro is also stuck. (Huh. Trying this again, it does enable Landscape lock now o_O). Let's assume that was me getting cross-eyed the first time (or missclicking somewhere :D).

Yes, that sounds like a misclick, as I didn't see anything which would cause this to happen.

pgaskin commented 4 years ago

Since the icon matches the usual "manual" lock state, it vaguely makes sense to me that the behavior should match too ;).

Yes, I guess there wouldn't usually be much reason (if any) to want to force an upside-down display if the device is upright.

That said, I'm absolutely not the guy to ask about actual experience with this: I pretty much do everything in Portrait ^^.

I can confirm that's how it would work, as I noticed it during my testing and while looking at libnickel.

I usually do stuff in Portrait unless I'm reading something landscape or I'm writing a webapp for the browser (it's usually a lot more useful in landscape). For me, my main use for this action is inverted portrait when I want to hold it from the top side or when I have a cable plugged into it.

Regarding the fully-locked landscape/portrait modes, do you think it'd be a good idea to add an option to allow gyro inversions or vice versa?

NiLuJe commented 4 years ago

Regarding the fully-locked landscape/portrait modes, do you think it'd be a good idea to add an option to allow gyro inversions or vice versa?

Definitely (c.f., https://github.com/pgaskin/NickelMenu/pull/70#issuecomment-669509319) ;).

And since I apparently got ghost email notifications for stuff I can't actually see (the GH website has been wonky as hell this week): that was on a Forma ;).

pgaskin commented 4 years ago

And since I apparently got ghost email notifications for stuff I can't actually see (the GH website has been wonky as hell this week): that was on a Forma ;).

That comment was in the review above 😉 . I got caught by that bug with auto-scrolling to the comment yesterday.

Speaking of bugs in GitHub, the cache has been very wonky recently. Also, when switching between desktop/mobile views, it seems to take you to the previous page you were on. And, references like #70 appear to be triggered in code blocks sporadically (a \ doesn't even escape them properly, but this shouldn't be needed in the first place), and GH support can't seem to reproduce it consistently.

Definitely (c.f., #70 (comment)) ;).

Which would be better: an option calledgyro_invert or rotations modes called locked_landscape/locked_portrait ?

Edit: I just remembered one of the reasons why I decided to fully lock it. I haven't gone through everything, but it appears some gyro settings are changed when switching between auto and one of the other locked views. We can't easily reproduce these changes, and I'd rather not have to walk QObjects (see the comments in the code) to get the required object to call the proper change methods on. I might leave this for later if there is demand for it.

pgaskin commented 4 years ago

If there aren't any further objections or possible issues/unexpected behavior, I'll merge this later today.

I'm going to put aside the idea for options to change to the usual locked orientations (where it can still invert from the gyro) for now, as they add significant complexity and/or possible issues which I have no way of testing myself. If there's demand for it, I might consider implementing them. @NiLuJe, one last question: if you use nickel_orientation:landscape and then press the Locked Landscape option in the menu (without switching to portrait/auto first or changing views), does it allow you to invert it with the gyro?

NiLuJe commented 4 years ago

@NiLuJe, one last question: if you use nickel_orientation:landscape and then press the Locked Landscape option in the menu (without switching to portrait/auto first or changing views), does it allow you to invert it with the gyro?

Yep, it does :).

pgaskin commented 4 years ago

Yep, it does :).

That's good. I guess this is ready to be merged now.