Skip to content

[ScreenSaver] handle case where a file is lastfile but has now been reset - #13747

Merged
Frenzie merged 4 commits into
koreader:masterfrom
Commodore64user:lastfile-reset
May 15, 2025
Merged

Frenzie merged 4 commits into
koreader:masterfrom
Commodore64user:lastfile-reset

Conversation

@Commodore64user

@Commodore64user Commodore64user commented May 7, 2025 •

Copy link
Copy Markdown
Member

bug fix:

  • Added a new flag is_document_cover to track whether the screensaver is displaying a document cover (frontend/ui/screensaver.lua).
  • Updated the logic to reset lastfile and disable show_message if the last opened file has been marked as unopened and the screensaver is not displaying a document cover (frontend/ui/screensaver.lua).

This change is Reviewable

Comment thread frontend/ui/screensaver.lua Outdated
Comment on lines +500 to +504
if not self.is_document_cover and lastfile then
-- book was the last file opened but has now been reset (i.e marked unopened)
lastfile = nil
self.show_message = false
end

@Frenzie Frenzie May 7, 2025 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems difficult to follow. I thought you'd simply rewrite it a little under the if lastfile where all the logic is located, along these general lines for example:

        if lastfile then
            local exclude_finished_book
            local exclude_on_hold_book
            local exclude_book_in_fm = not ui and G_reader_settings:isTrue("screensaver_hide_cover_in_filemanager")
            if BookList.hasBookBeenOpened(lastfile) then
                local doc_settings
                if ui and ui.doc_settings then
                    doc_settings = ui.doc_settings
                else
                    doc_settings = BookList.getDocSettings(lastfile)
                end
                excluded = doc_settings:isTrue("exclude_screensaver")

                local book_summary = doc_settings:readSetting("summary")
                local book_finished = book_summary and book_summary.status == "complete"
                local book_on_hold = book_summary and book_summary.status == "abandoned"
                exclude_finished_book = G_reader_settings:isTrue("screensaver_exclude_finished_books") and book_finished
                exclude_on_hold_book = G_reader_settings:isTrue("screensaver_exclude_on_hold_books") and book_on_hold
            end
            local should_exclude_book = exclude_book_in_fm or exclude_finished_book or exclude_on_hold_book
            if should_exclude_book then
                excluded = true
                self.show_message = false
            end

@Commodore64user Commodore64user May 7, 2025 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Question: regardless of exclude_book_in_fm if you reset a book (which was lastfile) and lock the device straight away, would you still expect to see that book cover and perhaps a message that will look like page 0 of 0 on the sleep screen?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would, but as long as it doesn't crash I'm presumably also fine with whatever it currently does instead. What does it do?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It falls into the else and shows the cover and the message as page 0 of 0, but i think that's an oversight rather than specifically meant that way. Which is why i went this way to catch that case as well

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To me that very much sounds like the intended behavior then.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You choose last file as screensaver or you don't. The last file is the last file. apple_x should either select a custom cover image or stop worrying.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure about that? I still firmly believe that going by the comment, that else didn't consider that specific case. It just so happens to fall in it. I'm just looking out for apple_x or any other fruit-letter pair combination ;)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I'd rather refrain from repeating the tautology yet again, but the last file is the last file.

The comment hold no relevance to the current logic, and if it did the logic would be wrong. When the comment was written the logic was if DocSettings says to exclude this book then exclude it. No DocSettings means no exclusion. That was changed in #13068, where we now know the logic should've been changed along the lines described in https://github.com/koreader/koreader/pull/13747/files#r2077285877 and the comment should've been deleted.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some other fruit-letter pair combination has no real opinion on what he could grasp after a really quick reading of the above.

@Commodore64user Commodore64user May 8, 2025 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

so you agree by omission, poi apple_x gets found out about his 'saucy' reading choices and you are okay with that ;)

Comment thread frontend/ui/screensaver.lua Outdated
local book_on_hold = book_summary and book_summary.status == "abandoned"
local exclude_finished_book = G_reader_settings:isTrue("screensaver_exclude_finished_books") and book_finished
local exclude_on_hold_book = G_reader_settings:isTrue("screensaver_exclude_on_hold_books") and book_on_hold
if not self.is_document_cover and lastfile then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is the only place is_document_cover is used I figure it should just be a regular local?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

only here yes, will chage it then.

Comment thread frontend/ui/screensaver.lua Outdated
local exclude_finished_book = G_reader_settings:isTrue("screensaver_exclude_finished_books") and book_finished
local exclude_on_hold_book = G_reader_settings:isTrue("screensaver_exclude_on_hold_books") and book_on_hold
if not self.is_document_cover and lastfile then
local exclude_finished_book, exclude_on_hold_book

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do find this pleasant to read but from a potential future change perspective separate lines tend to work nicer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not sure why this is a problem but okay

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Otherwise if you add or remove one the line changes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I’ve lost authorship of more important lines (unfairly sometimes), this one is not one i care much about ;)

@Commodore64user
Commodore64user requested a review from Frenzie May 15, 2025 19:59
@Frenzie
Frenzie merged commit af1829a into koreader:master May 15, 2025
@Frenzie Frenzie added this to the 2025.05 milestone May 15, 2025
@Commodore64user
Commodore64user deleted the lastfile-reset branch May 15, 2025 22:36
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
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.

3 participants