Dolphin: make --select scroll to the item need to be selected
Review Request #106209 - Created Aug. 26, 2012 and submitted
dolphin and konqueror provides "--select" to select items in arguments, but its not very useful since it doesn't scroll to the corresponding item. This patch is supposed to fix this problem.
No problem here and work as expected.
Thanks for the patch, looks good! One idea to improve it: It looks like DolphinMainWindow::markUrlAsCurrent() would not be needed any more if your patch is applied. Therefore, I would suggest to not add a new function DolphinView::markUrlNeedToScrollTo(), but just replace the line "m_currentItemUrl = url;" in markUrlAsCurrent() by "observeCreatedItem(url);".
Thanks for the new patch! I noticed that there is a small problem which I overlooked. When your patch is applied and loading the folder is finished, such that the file item model emits directoryLoadingCompleted(), two things happen: 1. DolphinView::selectAndScrollToCreatedItem() is invoked, makes the first URL passed on the command line after "--select" the current item and selects this item. 2. DolphinView::slotDirectoryLoadingCompleted() is invoked and calls DolphinView::updateViewState() via a single-shot timer. This function then selects *all* URLs passed on the command line after "--select". This results in a small, but noticeable delay between the selection of the first URL and the other URLs after "--select". Therefore, I'm wondering if it might be better to change the code around m_currentItemUrl and m_createdItemUrl more radically, because I think that having the directoryLoadingCompleted() signal invoke two different slots in DolphinView, which do different and partially competing things, is not optimal (which is not your fault, of course, this issue is already in the existing code). How about this: a) Add a new bool m_scrollToCurrentItem (best put just after m_currentItemUrl), which defaults to false. b) In markUrlAsCurrent(), set m_currentItemUrl to "url" and make m_scrollToCurrentItem true. c) In DolphinView::updateViewState(), just after selectionManager->setCurrentItem(currentIndex), scroll to the current item if m_scrollToCurrentItem is true and set it to false then. d) In DolphinView::observeCreatedItem(), just do the following: markUrlAsCurrent(url); markUrlsAsSelected(QList<KUrl>() << url); e) Remove m_createdItemUrl and DolphinView::selectAndScrollToCreatedItem(). What do you think about this? It's just an idea, and I haven't actually tried it yet, but I think that this should fix the small delay, get rid of the partial code duplication in selectAndScrollToCreatedItem() and updateViewState(), and make the code a bit nicer overall. Don't get me wrong: I like your patch and I think that it's already a nice improvement compared to the current situation. I just think that the existing code is more complicated than it needs to be, and I that this is a good opportunity to make it better.
Thanks, looks good! I like patches that simplify code and fix bugs at the same time :-) From my point of view, this can be pushed to the 4.9 branch - I don't think that David has objections about the DolphinPart changes. One bug I noticed when I tried your patch is that selecting items created with the "Create New..." menu does not work reliably in Konqueror, but this is unrelated to your patch, I can even reproduce that in KDE 4.8.