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

Do flashui refresh for first display of History page #11603

Merged
merged 2 commits into from Apr 6, 2024

Conversation

dmalinovsky
Copy link
Contributor

@dmalinovsky dmalinovsky commented Mar 28, 2024

This fixes ghosting for color Pocketbook devices

Needs koreader/koreader-base#1755.
Fixes #11602.


This change is Reviewable

@dmalinovsky
Copy link
Contributor Author

@hius07, does it look okay to you?

@hius07
Copy link
Member

hius07 commented Mar 30, 2024

@NiLuJe is in charge of this.

@NiLuJe
Copy link
Member

NiLuJe commented Mar 30, 2024

Not sure if that's the way to go, as I imagine it's mostly an issue with large mosaic views, not necessarily with history itself : it just happens to defaults to large mosaic views ;).

@NiLuJe
Copy link
Member

NiLuJe commented Mar 30, 2024

Unless the issue is mainly with view swaps? In which case, sure, that works. IIRC, the FM does something similar on initial display?

@dmalinovsky
Copy link
Contributor Author

Unless the issue is mainly with view swaps? In which case, sure, that works. IIRC, the FM does something similar on initial display?

Yes, FM does the flash refresh on the initial load and doesn’t have ghosting.

As you mentioned, this change affects first History load only, so it behaves similar to File Manager.

@NiLuJe
Copy link
Member

NiLuJe commented Apr 2, 2024

Cool, fine with me then ;).

@Frenzie Frenzie added this to the 2024.04 milestone Apr 2, 2024
@poire-z
Copy link
Contributor

poire-z commented Apr 4, 2024

I thought I would be bothered by this (I usually don't like flashes, and am fine with getting some bits of ghosting and having to do a small diagonal swipe to refresh when I want it).
But I'm actually not :) because I don't see any effect - because flashui is downgraded to ui when Screen > E-ink settings > [x] Avoid mandatory black flashes in UI is checked, as I have it.
So, fine with me.

BUT then, I don't think it's really what should be used. flashui is only used in a few widgets, mostly in onCloseWidget, and only onShow with DictQuickLookup (and for some tricky stuff in VirtualKeyboard and InputDialog).
It feels like a "partial" (or "full"?) should be triggered when we are in a mode with (large only?) covers, and not when mostly text.
But if @NiLuJe feels "flashui" is fine, and that this History (so, only it, not Favorites or File browser) can use it (so it can be oddly avoided with [x] Avoid mandatory black flashes in UI), fine with me too.

@dmalinovsky
Copy link
Contributor Author

It feels like a "partial" (or "full"?) should be triggered when we are in a mode with (large only?) covers, and not when mostly text.

Yes, since the default History mode display large covers, this change reduces ghosting for color Pocketbooks.

@hius07
Copy link
Member

hius07 commented Apr 5, 2024

Again, I see no differences in the code showing History and Favorites. Can we investigate why do they look differently in color?

@dmalinovsky
Copy link
Contributor Author

Again, I see no differences in the code showing History and Favorites. Can we investigate why do they look differently in color?

Well, I suspect it’s because Favorites use small thumbnails unlike History and ghosting is less visible there.

@hius07
Copy link
Member

hius07 commented Apr 5, 2024

Favorites use small thumbnails unlike History

It depends on your settings, they can be adjusted separately, and there is a checkbox to unify FM, Hist, Fav display modes.

@dmalinovsky
Copy link
Contributor Author

It depends on your settings, they can be adjusted separately, and there is a checkbox to unify FM, Hist, Fav display modes.

Okay, to be sure, I've tested FM and Favorites in the cover display mode. They both do flash refresh before first display.

@hius07
Copy link
Member

hius07 commented Apr 5, 2024

It's interesting because Hist and Fav can be shown over the Reader in the same way:

UIManager:show(self.hist_menu)

UIManager:show(self.coll_menu)

Anyway, if it is good on a color device, I'm fine with this PR.

dmalinovsky and others added 2 commits April 6, 2024 13:06
This fixes ghosting for color Pocketbook devices

Fixes koreader#11602.
@Frenzie Frenzie merged commit 6cc970d into koreader:master Apr 6, 2024
1 of 3 checks passed
@poire-z
Copy link
Contributor

poire-z commented Apr 6, 2024

I still think this was not the right solution.
I would have liked to know what triggers a flash or proper refresh in the other cases, and see if we can do the same.
Just sticking a "flashui" that is otherwise never used in such contexts is a bit too quick.
I'm sure if @NiLuJe would have had the time, he would have found a better solution.
(And I fear that being absent for so long, if one day you are back, which I hope you will... - you'll have a bunch of cleanup to do :/).

(The ghosting is not only for color devices: if I have some dark/black covers, when launching History over my book text, I see light gray remnants of the book text in the black covers.)

@NiLuJe
Copy link
Member

NiLuJe commented Apr 6, 2024

you'll have a bunch of cleanup to do :/

I already had a bunch of tabs open to remind me anyway ;p.


I would have liked to know what triggers a flash or proper refresh in the other cases, and see if we can do the same.

I would tend to agree with that, but please define "other cases" anyway ;).

Your previous comment also reminded me that flashui is conditional on the "avoid mandatory flashes" eInk setting, so, if we do indeed want a flash, and given than this is obviously always a full-screen affair, a full makes more sense.

FWIW, on paper, I think a flash on a view swap sort of makes sense, and it's probably easier to handle it on show than on close here.

The ghosting is not only for color devices

Yeah, since I've had to switch out my Forma for the Clara 2E as my "on the go" device, and having to use it significantly more than expected, turns out some devices have really really crappy REAGL implementations, even recent ones ;p.

@NiLuJe
Copy link
Member

NiLuJe commented Apr 6, 2024

Your previous comment also reminded me that flashui is conditional on the eInk settings, so, if we do indeed want a flash, and given than this is obviously always a full-screen affair, a full makes more sense.

Then again, while it's true I originally envisioned flashui as only being used for partially occluding UI elements, we do have a few full-screen ones in the current codebase, and honoring user settings in that respect and in this specific case could be a good thing, given that the current behavior is utterly unproblematic on most devices?

Sidebar: Suffering from the mother of all head colds for a week now, so brain is pretty mushy, I may not be making much sense ^^.

@Frenzie
Copy link
Member

Frenzie commented Apr 7, 2024

Don't overwork yourself. ;)

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.

History page should do flashui refresh on first display
5 participants