Re: Review Request 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

2015-12-14 Thread Xuetian Weng 

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

(Updated Dec. 14, 2015, 11:31 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.


Changes
---

Submitted with commit d34b37b6831429f7a48edec7de77cb88524b3784 by Weng Xuetian 
to branch master.


Repository: kio


Description
---

9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on startIndex. 
The argument index passed to buttonUrl is based number of '/'in full url. 
Because of that, url like "sftp://a...@b.com/a/b; is now shown as 
"sftp:a...@b.com" / "b" / "b" in dolphin url bar.

This patch changes all relevant code that calls buttonUrl() to use url.path() 
instead of url.toDisplayString() to count the number of "/".


Diffs
-

  src/filewidgets/kurlnavigator.cpp 848e98b 

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


Testing
---

Remote url -> ok
Local url -> ok
Remote url in places and try browse some sub folder -> ok
Local url in places and try browse some sub folder -> ok


Thanks,

Xuetian Weng

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

Re: Review Request 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

2015-12-12 Thread Xuetian Weng 

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

(Updated Dec. 12, 2015, 9:35 a.m.)


Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.


Repository: kio


Description
---

9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on startIndex. 
The argument index passed to buttonUrl is based number of '/'in full url. 
Because of that, url like "sftp://a...@b.com/a/b; is now shown as 
"sftp:a...@b.com" / "b" / "b" in dolphin url bar.

This patch changes all relevant code that calls buttonUrl() to use url.path() 
instead of url.toDisplayString() to count the number of "/".


Diffs (updated)
-

  src/filewidgets/kurlnavigator.cpp 848e98b 

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


Testing
---

Remote url -> ok
Local url -> ok
Remote url in places and try browse some sub folder -> ok
Local url in places and try browse some sub folder -> ok


Thanks,

Xuetian Weng

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

Re: Review Request 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

2015-12-12 Thread David Faure 

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



src/filewidgets/kurlnavigator.cpp (line 156)


Please rename to retrievePlaceUrl, otherwise when looking at the calling 
code it still seems like it returns a path ;)



src/filewidgets/kurlnavigator.cpp (line 368)


The issue was there already, but I don't see why this calls 
retrievePlacePath() again, it could just do placeUrl.toDisplayString...



src/filewidgets/kurlnavigator.cpp (line 369)


This comment is misleading. That method doesn't return an empty url for 
local files. The path is empty, that's what you meant, right?

But then no point in calling toDisplayString(PreferLocalFile), if we know 
that for local files we'll need "/" anyway, right?
IIUC this could be simplified/clarified to

if (placeUrl.isLocalFile()) {
dirName = QStringLiteral("/");
} else {
dirName = placeUrl.toDisplayString();
}

no?



src/filewidgets/kurlnavigator.cpp (line 784)


setPath(QString()) slightly faster


- David Faure


On Dec. 12, 2015, 9:35 a.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126295/
> ---
> 
> (Updated Dec. 12, 2015, 9:35 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> 9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on 
> startIndex. The argument index passed to buttonUrl is based number of '/'in 
> full url. Because of that, url like "sftp://a...@b.com/a/b; is now shown as 
> "sftp:a...@b.com" / "b" / "b" in dolphin url bar.
> 
> This patch changes all relevant code that calls buttonUrl() to use url.path() 
> instead of url.toDisplayString() to count the number of "/".
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigator.cpp 848e98b 
> 
> Diff: https://git.reviewboard.kde.org/r/126295/diff/
> 
> 
> Testing
> ---
> 
> Remote url -> ok
> Local url -> ok
> Remote url in places and try browse some sub folder -> ok
> Local url in places and try browse some sub folder -> ok
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

2015-12-12 Thread Xuetian Weng 

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

(Updated Dec. 12, 2015, 11:11 a.m.)


Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.


Repository: kio


Description
---

9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on startIndex. 
The argument index passed to buttonUrl is based number of '/'in full url. 
Because of that, url like "sftp://a...@b.com/a/b; is now shown as 
"sftp:a...@b.com" / "b" / "b" in dolphin url bar.

This patch changes all relevant code that calls buttonUrl() to use url.path() 
instead of url.toDisplayString() to count the number of "/".


Diffs (updated)
-

  src/filewidgets/kurlnavigator.cpp 848e98b 

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


Testing
---

Remote url -> ok
Local url -> ok
Remote url in places and try browse some sub folder -> ok
Local url in places and try browse some sub folder -> ok


Thanks,

Xuetian Weng

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

Re: Review Request 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

2015-12-12 Thread David Faure 

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

Ship it!


We could findChildren in an autotest to find the buttons and check their number 
and the text on each one.


src/filewidgets/kurlnavigator.cpp (line 371)


No need for PreferLocalFile anymore, the url is always remote. My code was 
correct :)


- David Faure


On Dec. 12, 2015, 11:11 a.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126295/
> ---
> 
> (Updated Dec. 12, 2015, 11:11 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> 9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on 
> startIndex. The argument index passed to buttonUrl is based number of '/'in 
> full url. Because of that, url like "sftp://a...@b.com/a/b; is now shown as 
> "sftp:a...@b.com" / "b" / "b" in dolphin url bar.
> 
> This patch changes all relevant code that calls buttonUrl() to use url.path() 
> instead of url.toDisplayString() to count the number of "/".
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigator.cpp 848e98b 
> 
> Diff: https://git.reviewboard.kde.org/r/126295/diff/
> 
> 
> Testing
> ---
> 
> Remote url -> ok
> Local url -> ok
> Remote url in places and try browse some sub folder -> ok
> Local url in places and try browse some sub folder -> ok
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

2015-12-10 Thread David Faure 

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



src/filewidgets/fix-url-navigator.patch (line 1)


I don't think you wanted to added this patch to the git repo :-)



src/filewidgets/kurlnavigator.cpp (line 361)


This can be a local path, you can't give it to the QUrl(QString) 
constructor (it would break e.g. when the filename contains '#')

It seems retrievePlacePath() can return either a local path or a URL. I 
would change that to always return a URL, that's much easier to handle 
programmatically. Use toDisplayString(PreferLocalFile) only at the time of 
displaying this to the user.
retrievePlacePath() doing idx+=3 and url.left(idx) is indeed very very 
wrong, in terms of path escaping.


- David Faure


On Dec. 9, 2015, 9:42 p.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126295/
> ---
> 
> (Updated Dec. 9, 2015, 9:42 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> 9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on 
> startIndex. The argument index passed to buttonUrl is based number of '/'in 
> full url. Because of that, url like "sftp://a...@b.com/a/b; is now shown as 
> "sftp:a...@b.com" / "b" / "b" in dolphin url bar.
> 
> This patch changes all relevant code that calls buttonUrl() to use url.path() 
> instead of url.toDisplayString() to count the number of "/".
> 
> 
> Diffs
> -
> 
>   src/filewidgets/fix-url-navigator.patch PRE-CREATION 
>   src/filewidgets/kurlnavigator.cpp 848e98b 
> 
> Diff: https://git.reviewboard.kde.org/r/126295/diff/
> 
> 
> Testing
> ---
> 
> Remote url -> ok
> Local url -> ok
> Remote url in places and try browse some sub folder -> ok
> Local url in places and try browse some sub folder -> ok
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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