Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2017-07-30 Thread Gregor Mi 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/
---

(Updated July 30, 2017, 5:32 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Emmanuel Pescosta and Frank Reininghaus.


Changes
---

Submitted with commit fa6bad3be579ec89c8126b00438234d0bc16c508 by Gregor Mi to 
branch master.


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
-

  autotests/CMakeLists.txt 83b7b73b4b92e09076ece2d4618559ddb8089368 
  autotests/urlutiltest.cpp PRE-CREATION 
  src/filewidgets/kurlnavigator.h ff155c7bbdc8c72f579f730993286a4dccae6338 
  src/filewidgets/kurlnavigator.cpp 033046f06dd5bea3f4124669c55803aba3a31789 
  src/filewidgets/urlutil_p.h PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127111/diff/


Testing
---


Thanks,

Gregor Mi

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2017-07-28 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/#review103504
---


Fix it, then Ship it!





src/filewidgets/kurlnavigator.h (line 432)


5.37 by now


- David Faure


On July 27, 2017, 11:15 a.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127111/
> ---
> 
> (Updated July 27, 2017, 11:15 a.m.)
> 
> 
> Review request for 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
> -
> 
>   autotests/CMakeLists.txt 83b7b73b4b92e09076ece2d4618559ddb8089368 
>   autotests/urlutiltest.cpp PRE-CREATION 
>   src/filewidgets/kurlnavigator.h ff155c7bbdc8c72f579f730993286a4dccae6338 
>   src/filewidgets/kurlnavigator.cpp 033046f06dd5bea3f4124669c55803aba3a31789 
>   src/filewidgets/urlutil_p.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127111/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2017-07-27 Thread Gregor Mi 


> On Nov. 20, 2016, 9:52 p.m., David Faure wrote:
> > I forgot to review this, sorry for the delay.

Sorry my own delay. Thanks for the detailled review. Everything should be fixed 
now.


- Gregor


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/#review100988
---


On July 27, 2017, 11:15 a.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127111/
> ---
> 
> (Updated July 27, 2017, 11:15 a.m.)
> 
> 
> Review request for 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
> -
> 
>   autotests/CMakeLists.txt 83b7b73b4b92e09076ece2d4618559ddb8089368 
>   autotests/urlutiltest.cpp PRE-CREATION 
>   src/filewidgets/kurlnavigator.h ff155c7bbdc8c72f579f730993286a4dccae6338 
>   src/filewidgets/kurlnavigator.cpp 033046f06dd5bea3f4124669c55803aba3a31789 
>   src/filewidgets/urlutil_p.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127111/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2017-07-27 Thread Gregor Mi 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/
---

(Updated July 27, 2017, 11:15 a.m.)


Review request for KDE Frameworks, Emmanuel Pescosta and Frank Reininghaus.


Changes
---

* Rename urlutil.h to  urlutil_p.h
* Remove include QDebug which is no longer used
* Rename local variables to better reflect the meaning (childPath, parentPath)
* Remove confusing or useless comments; fix other comments
* unit test: Move urlutil test from tests to autotest
* unit test: Replace ugly _LURL_ preprocessor macro with inline function
* unit test: remove plain wrong call of QUrl::fromLocalFile with a path 
containing an URL scheme
* unit test: add a test with # in the path to find potential path/url confusion 
in the code
* unit test: all tests pass


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 (updated)
-

  autotests/CMakeLists.txt 83b7b73b4b92e09076ece2d4618559ddb8089368 
  autotests/urlutiltest.cpp PRE-CREATION 
  src/filewidgets/kurlnavigator.h ff155c7bbdc8c72f579f730993286a4dccae6338 
  src/filewidgets/kurlnavigator.cpp 033046f06dd5bea3f4124669c55803aba3a31789 
  src/filewidgets/urlutil_p.h PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127111/diff/


Testing
---


Thanks,

Gregor Mi

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-11-20 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/#review100988
---



I forgot to review this, sorry for the delay.


src/filewidgets/urlutil.h (line 20)


This file isn't installed so it should be renamed to urlutil_p.h



src/filewidgets/urlutil.h (line 24)


no longer used



src/filewidgets/urlutil.h (line 65)


The len=5 in the comment is strange, given that this code doesn't look at 
lengths.

The /home==/home comment is strange too, because this code is hit in many 
many other cases too, like
/a and /b, or any other case where currentUrl isn't a parent of lastUrl.

I'd just remove the 3 lines of comments, the if() is clear enough.



src/filewidgets/urlutil.h (line 71)


If path2 requires a comment saying "the child one", how about renaming 
path2 to childPath and path1 to parentPath? :-)



src/filewidgets/urlutil.h (line 92)


// keeps the scheme

would be a more correct comment.

It could be file:// or anything else, but for sure there is one, we only 
work with absolute URLs in KIO.



src/filewidgets/urlutil.h (line 93)


remove useless comment ;)



tests/CMakeLists.txt (line 53)


yes but you should move this to the autotests subfolder.

"tests" is for interactive test programs.

And there you'll also be able to integrate better with the cmakelists for 
the other autotests ;-)



tests/urlutiltest.cpp (line 32)


Using the pre-processor is ugly and unnecessary here. Make this an inline 
function instead?

static inline QUrl lUrl(const QString &path) { return 
QUrl::fromLocalFile(path); }



tests/urlutiltest.cpp (line 44)


Calling QUrl::fromLocalFile("file:///home/aaa") is just plain wrong, either 
remove the scheme or don't call fromLocalFile.

OTOH you should add tests with '#' in the path, since that would hit any 
path/url confusion in the code.


- David Faure


On Sept. 6, 2016, 9:59 a.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127111/
> ---
> 
> (Updated Sept. 6, 2016, 9:59 a.m.)
> 
> 
> Review request for 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 3c045c5aaadb429fd22d28b55ad20d31b086ef48 
>   src/filewidgets/urlutil.h PRE-CREATION 
>   tests/CMakeLists.txt bc94a4aefd77fb9ce9b2a24075415aac7c30e5ca 
>   tests/urlutiltest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127111/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-09-20 Thread Emmanuel Pescosta 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/#review99295
---



Looks good to me!

- Emmanuel Pescosta


On Sept. 6, 2016, 11:59 a.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127111/
> ---
> 
> (Updated Sept. 6, 2016, 11:59 a.m.)
> 
> 
> Review request for 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 3c045c5aaadb429fd22d28b55ad20d31b086ef48 
>   src/filewidgets/urlutil.h PRE-CREATION 
>   tests/CMakeLists.txt bc94a4aefd77fb9ce9b2a24075415aac7c30e5ca 
>   tests/urlutiltest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127111/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-09-06 Thread Gregor Mi 


> On Feb. 20, 2016, 7:46 p.m., David Faure wrote:
> > src/filewidgets/kurlnavigator.h, line 425
> > 
> >
> > I had to read the whole patch to understand "url is set to the first 
> > path segment that leads from N to O".
> > 
> > First, N and O don't help readability, I first thought the O was 0... 
> > maybe just spell out "the new url" and "the old url".
> > 
> > Also, a url is not a path. So this could say something like:
> > 
> > \a url is set to the child directory of the new url which is an 
> > ancestor of the old url
> > 
> > I would then add "this signal allows file managers to pre-select the 
> > directory that the user is navigating up from".

That sounds better. Fixed in the coming update.


- Gregor


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/#review92587
---


On Sept. 6, 2016, 9:59 a.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127111/
> ---
> 
> (Updated Sept. 6, 2016, 9:59 a.m.)
> 
> 
> Review request for 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 3c045c5aaadb429fd22d28b55ad20d31b086ef48 
>   src/filewidgets/urlutil.h PRE-CREATION 
>   tests/CMakeLists.txt bc94a4aefd77fb9ce9b2a24075415aac7c30e5ca 
>   tests/urlutiltest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127111/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-09-06 Thread Gregor Mi 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/
---

(Updated Sept. 6, 2016, 9:59 a.m.)


Review request for KDE Frameworks, Emmanuel Pescosta and Frank Reininghaus.


Changes
---

- rebase to latest master
- rename signal to urlSelectionRequested
- improve API documentation


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 (updated)
-

  src/filewidgets/kurlnavigator.h 4ffe56acf9ef7a80ba27ba3a08327e363f98fb0d 
  src/filewidgets/kurlnavigator.cpp 3c045c5aaadb429fd22d28b55ad20d31b086ef48 
  src/filewidgets/urlutil.h PRE-CREATION 
  tests/CMakeLists.txt bc94a4aefd77fb9ce9b2a24075415aac7c30e5ca 
  tests/urlutiltest.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/127111/diff/


Testing
---


Thanks,

Gregor Mi

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-08-12 Thread Gregor Mi 


> On Feb. 20, 2016, 7:46 p.m., David Faure wrote:
> > src/filewidgets/kurlnavigator.h, line 428
> > 
> >
> > 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 ;)
> 
> David Faure wrote:
> Gregor, do you plan on updating the patch based on the above conclusions? 
> Thanks.

Yes, I do. It is still on my list. Sorry for not to have responded earlier.


- Gregor


---
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
> 
>

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-08-12 Thread David Faure 


> On Feb. 20, 2016, 7:46 p.m., David Faure wrote:
> > src/filewidgets/kurlnavigator.h, line 428
> > 
> >
> > 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
> 
>

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-02-23 Thread David Faure 


> On Feb. 20, 2016, 7:46 p.m., David Faure wrote:
> > src/filewidgets/kurlnavigator.h, line 428
> > 
> >
> > 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 &) ?

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 ;)


- 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-02-22 Thread Frank Reininghaus 


> On Feb. 20, 2016, 7:46 p.m., David Faure wrote:
> > src/filewidgets/kurlnavigator.h, line 428
> > 
> >
> > 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.

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 &) ?


- Frank


---
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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-02-20 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/#review92587
---




src/filewidgets/kurlnavigator.h (line 425)


I had to read the whole patch to understand "url is set to the first path 
segment that leads from N to O".

First, N and O don't help readability, I first thought the O was 0... maybe 
just spell out "the new url" and "the old url".

Also, a url is not a path. So this could say something like:

\a url is set to the child directory of the new url which is an ancestor of 
the old url

I would then add "this signal allows file managers to pre-select the 
directory that the user is navigating up from".



src/filewidgets/kurlnavigator.h (line 428)


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.


- David Faure


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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-02-18 Thread Gregor Mi 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127111/
---

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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel