Scroll to item if it's not visible on keyPress
ClosedPublic

Authored by trmdi on Apr 1 2019, 4:06 AM.

Details

Summary

Always scroll to item if it's not visible on keyPress, even if it's the current index.

Test Plan

Select the last item, scroll up to make it not visible > press the End key > Dolphin now scrolls to that item.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
trmdi created this revision.Apr 1 2019, 4:06 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptApr 1 2019, 4:06 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
trmdi requested review of this revision.Apr 1 2019, 4:06 AM
ngraham accepted this revision.Apr 1 2019, 5:30 PM
ngraham added a subscriber: elvisangelaccio.

@elvisangelaccio does this look good? And if so, should we land it on the stable branch?

This revision is now accepted and ready to land.Apr 1 2019, 5:30 PM
elvisangelaccio requested changes to this revision.Apr 7 2019, 4:58 PM

We always call KItemListSelectionManager::setCurrentItem() before calling scrollToItem(), so we should probably do it also here.

This revision now requires changes to proceed.Apr 7 2019, 4:58 PM
trmdi updated this revision to Diff 55719.Apr 8 2019, 3:58 AM

rebase

trmdi added a comment.EditedApr 8 2019, 4:00 AM

We always call KItemListSelectionManager::setCurrentItem() before calling scrollToItem(), so we should probably do it also here.

I don't think it is needed because it was set in the above code block.

if (m_selectionManager->currentItem() != index) {
    ...
    m_selectionManager->setCurrentItem(index);
    ...
}

if (index < m_view->firstVisibleIndex() || index > m_view->lastVisibleIndex()) {
    m_view->scrollToItem(index);
}

We always call KItemListSelectionManager::setCurrentItem() before calling scrollToItem(), so we should probably do it also here.

I don't think it is needed because it was set in the above code block.

Only if m_selectionManager->currentItem() != index, but this won't be true when the item is not visible. Which is why you want to move the scrollToItem() call outside this if() block in the first place, isn't it?

trmdi added a comment.EditedApr 8 2019, 9:17 PM

We always call KItemListSelectionManager::setCurrentItem() before calling scrollToItem(), so we should probably do it also here.

I don't think it is needed because it was set in the above code block.

Only if m_selectionManager->currentItem() != index, but this won't be true when the item is not visible. Which is why you want to move the scrollToItem() call outside this if() block in the first place, isn't it?

When m_selectionManager->currentItem() == index, the setCurrentItem(index) will do nothing.
See https://phabricator.kde.org/source/dolphin/browse/master/src/kitemviews/kitemlistselectionmanager.cpp$51

Before it is not visible, that setCurrentItem(index) function was called once before (must be called), and there is no change while we scroll up and press End again, so we don't need to call it again.

We always call KItemListSelectionManager::setCurrentItem() before calling scrollToItem(), so we should probably do it also here.

I don't think it is needed because it was set in the above code block.

Only if m_selectionManager->currentItem() != index, but this won't be true when the item is not visible. Which is why you want to move the scrollToItem() call outside this if() block in the first place, isn't it?

When m_selectionManager->currentItem() == index, the setCurrentItem(index) will do nothing.

Fair enough. But anyway I found a bigger problem: with this patch we will scroll unconditionally, even when pressing unrelated keys such as Key_Escape or Key_Menu.

trmdi updated this revision to Diff 55870.Apr 10 2019, 3:31 AM

Scroll only when a navigation key is pressed

ngraham accepted this revision.Apr 10 2019, 4:40 PM

Thanks, that works well now for me.

elvisangelaccio accepted this revision.Apr 11 2019, 5:32 PM

Please push to master only, just to make sure we don't introduce regressions.

This revision is now accepted and ready to land.Apr 11 2019, 5:32 PM
This revision was automatically updated to reflect the committed changes.
Zren added a subscriber: Zren.Tue, Jun 4, 12:19 AM

This commit broke the PgUp and PgDn keys for me. They select, but no longer scroll. It also stops scrolling when using the up arrow key before reaching the top.

trmdi added a comment.Tue, Jun 4, 9:00 AM

Sorry, I sent a new patch to fix this: https://phabricator.kde.org/D21573
Please review it.