Fix going to prev/next article with keyboard when using load full website
ClosedPublic

Authored by thomasbrixlarsen on Apr 23 2018, 5:26 PM.

Details

Summary

Articles were loaded three times when keyboard selection was used. This broke rendering so the selected article was never rendered.
This is a regression as keyboard selection was working one or two releases ago.

Test Plan

Set feed to use rendering of full website. Press left/right arrow. See different article displayed.

Diff Detail

Repository
R201 Akregator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a subscriber: KDE PIM. · View Herald TranscriptApr 23 2018, 5:26 PM
thomasbrixlarsen requested review of this revision.Apr 23 2018, 5:26 PM

Using left/right keys to change articles just works for me. Any special settings that's affecting this?

Are you using "Load the full website when reading articles"?

I just tested an RSS feed without "Load the full website when reading articles" no issue with left/right arrows.

thomasbrixlarsen retitled this revision from Fix going to prev/next article with keyboard to Fix going to prev/next article with keyboard when using load full website.Apr 25 2018, 4:54 PM
thomasbrixlarsen edited the test plan for this revision. (Show Details)
dvratil added a comment.EditedApr 25 2018, 8:10 PM

Man, I literally had to grep the codebase to find where that checkbox is. I had no idea about this feature :D Anyhow, I can reproduce it now (although only on certain feeds, some feeds, like planetkde.org, work)

I'm trying to understand how updating the selection might be related to the problem. Makes me think if maybe the real issue isn't somewhere else, maybe in the SelectionController

Haha yeah it's well hidden. Every time I add a new feed I wonder why it isn't the default.

Blame shows that these lines haven't been changed for 10-11 years. So the regression is obviously somewhere else. Yet I tracked the multiple page loads to here, perhaps it was always loading multiple times.

Ok, I couldn't see any other breakage with this change so +1 from me. Let's wait for the new Akregator maintainer @pinaraf .

pinaraf accepted this revision.Apr 30 2018, 4:25 PM

Nice catch. I'm wondering what was the use of this code 10 years ago.

This revision is now accepted and ready to land.Apr 30 2018, 4:25 PM
This revision was automatically updated to reflect the committed changes.