> On Feb. 20, 2016, 7:46 p.m., David Faure wrote: > > src/filewidgets/kurlnavigator.h, line 428 > > <https://git.reviewboard.kde.org/r/127111/diff/1/?file=444588#file444588line428> > > > > Signal names usually end with "ed", they reflect a state change or an > > action change. > > > > "select" here is what you want the app to do (now that I read the bug > > report; otherwise it was very unclear just from the API docs), but you have > > no guarantee that the app will do that. > > > > void urlChangedToParent(const QUrl &ancestorOfPreviousUrl) > > > > ? > > > > An alternative is to let the app do the "finding the immediate child" > > logic by just emitting > > > > void urlChangedToParent(const QUrl &oldUrl, const QUrl &newUrl) > > > > It seems to me that this is a better signal, because it's more generic. > > On the other hand, if you are afraid that multiple apps would then need to > > implement the "first child" logic, then this could be done by a helper > > method in this class. But at least the signal has a much more generic > > purpose than being geared towards this specific feature, > > which increases the chances that it is useful for other things later. > > Frank Reininghaus wrote: > I also thought first that something like urlChangedToParent(QUrl) would > be a good signal name, but it might make people think that this signal will > always be emitted when navigating from a URL to its (possibly indirect) > parent. > > However, this is not the case - the new signal will only be emitted if > the URL change of the KUrlNavigator was triggered by a call of > setLocationUrl(const QUrl&). If the URL change was caused by invoking the > goBack() or goForward() slots, then the signal will not be emitted. The > reason is that the user of KUrlNavigator will try to restore the view state > (using the result from KUrlNavigator's locationState() function) then, and > selecting any parents would interfere with that (see the discussion in > https://git.reviewboard.kde.org/r/123253/ ). > > So the idea is that the new signal is only emitted if no history action > was responsible for the URL change. This makes it possible for Dolphin and > other users of KUrlNavigator to behave like some other file managers, which > also select the parent of the previous URL, unless the URL change was caused > by back/forward. So anything with 'changed' is not really accurate, because a > change is not sufficient to emit this signal. > > Still, it might make sense to have an 'ed' in the name. 'requested' > appears often in signals which are not so much about state changes, but have > the purpose to make the receiver do something. How about > > urlSelectionRequested(const QUrl &) ? > > David Faure wrote: > OK, I see. Yes, this is better. The comment would still explain this use > case for requesting selection, but in theory this lets the door open for more > use cases for the url navigator requesting that a selection be performed. In > any case the signal name is much easier to understand this way ;)
Gregor, do you plan on updating the patch based on the above conclusions? Thanks. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127111/#review92587 ----------------------------------------------------------- On Feb. 18, 2016, 9:53 p.m., Gregor Mi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127111/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2016, 9:53 p.m.) > > > Review request for Dolphin, KDE Frameworks, Emmanuel Pescosta, and Frank > Reininghaus. > > > Bugs: 335616 > https://bugs.kde.org/show_bug.cgi?id=335616 > > > Repository: kio > > > Description > ------- > > Moved logic from https://git.reviewboard.kde.org/r/123253 to here. > > Provides a signal to implement bug > https://bugs.kde.org/show_bug.cgi?id=335616: "Dolphin: Navigate to parent > folder selects child folder". > > > Diffs > ----- > > src/filewidgets/kurlnavigator.h 4ffe56acf9ef7a80ba27ba3a08327e363f98fb0d > src/filewidgets/kurlnavigator.cpp 64d2a6d1d5cf8ca2e0aaa61d00ac1ffb1a9866b3 > src/filewidgets/urlutil.h PRE-CREATION > tests/CMakeLists.txt dc88ce9fd5c5ae6ad135b72785370c0094969b5c > tests/urlutiltest.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/127111/diff/ > > > Testing > ------- > > > Thanks, > > Gregor Mi > >
