D17270: [KUrlNavigator] List subdirs of a parent folder of an archive
elvisangelaccio added inline comments.
INLINE COMMENTS
> kurlnavigator.h:297
> + */
> +bool isInsideCompressedPath(const QUrl ) const;
> +
I'd make this function `static`, since it doesn't depend on the status of a
specific `KUrlNavigator` instance.
To do so, `isCompressedPath()` needs to become a (static) free function.
> kurlnavigatorbutton.cpp:416
> +auto kurlnavigator = qobject_cast(parent());
> +if
> (kurlnavigator->isInsideCompressedPath(kurlnavigator->locationUrl())) {
> +// We are in an archive, check whether the subdir we have to
> list is part of the archive
What if the parent is not a `KUrlNavigator`?
> thsurrel wrote in kurlnavigatorbutton.cpp:414
> What would be the clean way to expose this function ? I wouldn't like to
> duplicate the code.
> And btw, the "if ((m_url.scheme() == QLatin1String("tar")) || (m_url.scheme()
> == QLatin1String("zip")))" condition was taken from
> KUrlNavigator::setLocationUrl, so there must be a bugfix to be made there as
> well with the 'krarc' case gregormi reports.
> What would be the clean way to expose this function ? I wouldn't like to
> duplicate the code.
Sorry for the late reply. One way could be a public static function, either in
`KUrlNavigator` or `KUrlNavigatorButton`.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17270
To: thsurrel, #frameworks
Cc: elvisangelaccio, gregormi, kde-frameworks-devel, michaelh, ngraham, bruns
D17270: [KUrlNavigator] List subdirs of a parent folder of an archive
thsurrel updated this revision to Diff 47545. thsurrel added a comment. Create KUrlNavigator::isInsideCompressedPath Use this function instead of comparing the url scheme with tar, zip, ... REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17270?vs=46589=47545 BRANCH arc_urlnavigatorbutton (branched from master) REVISION DETAIL https://phabricator.kde.org/D17270 AFFECTED FILES src/filewidgets/kurlnavigator.cpp src/filewidgets/kurlnavigator.h src/filewidgets/kurlnavigatorbutton.cpp To: thsurrel, #frameworks Cc: elvisangelaccio, gregormi, kde-frameworks-devel, michaelh, ngraham, bruns
D17270: [KUrlNavigator] List subdirs of a parent folder of an archive
thsurrel added inline comments.
INLINE COMMENTS
> elvisangelaccio wrote in kurlnavigatorbutton.cpp:414
> Or we could use mimetype detection like we do in
> `KUrlNavigator::Private::isCompressedPath()`.
What would be the clean way to expose this function ? I wouldn't like to
duplicate the code.
And btw, the "if ((m_url.scheme() == QLatin1String("tar")) || (m_url.scheme()
== QLatin1String("zip")))" condition was taken from
KUrlNavigator::setLocationUrl, so there must be a bugfix to be made there as
well with the 'krarc' case gregormi reports.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17270
To: thsurrel, #frameworks
Cc: elvisangelaccio, gregormi, kde-frameworks-devel, michaelh, ngraham, bruns
D17270: [KUrlNavigator] List subdirs of a parent folder of an archive
elvisangelaccio added inline comments.
INLINE COMMENTS
> gregormi wrote in kurlnavigatorbutton.cpp:414
> When I click a zip file I get this in the URL bar:
>
> krarc:/home/gregor/Downloads/kfk_10p.zip/
>
> This could mean that "krarc" should be added to this if statement.
Or we could use mimetype detection like we do in
`KUrlNavigator::Private::isCompressedPath()`.
> kurlnavigatorbutton.cpp:417
> +// or is a parent directory.
> +QDir test_dir(url.path());
> +if (test_dir.exists()) {
Coding sytle: variable names need to be camelCase.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17270
To: thsurrel, #frameworks
Cc: elvisangelaccio, gregormi, kde-frameworks-devel, michaelh, ngraham, bruns
D17270: [KUrlNavigator] List subdirs of a parent folder of an archive
gregormi added inline comments.
INLINE COMMENTS
> kurlnavigatorbutton.cpp:414
> +url = KIO::upUrl(m_url);
> +} else if ((m_url.scheme() == QLatin1String("tar")) || (m_url.scheme()
> == QLatin1String("zip"))) {
> +// We are in an archive, check whether the subdir we have to list is
> part of the archive
When I click a zip file I get this in the URL bar:
krarc:/home/gregor/Downloads/kfk_10p.zip/
This could mean that "krarc" should be added to this if statement.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17270
To: thsurrel, #frameworks
Cc: gregormi, kde-frameworks-devel, michaelh, ngraham, bruns
D17270: [KUrlNavigator] List subdirs of a parent folder of an archive
thsurrel updated this revision to Diff 46589. thsurrel added a comment. Fix deleted empty line. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17270?vs=46588=46589 BRANCH arc_urlnavigatorbutton (branched from master) REVISION DETAIL https://phabricator.kde.org/D17270 AFFECTED FILES src/filewidgets/kurlnavigatorbutton.cpp To: thsurrel, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D17270: [KUrlNavigator] List subdirs of a parent folder of an archive
thsurrel created this revision. thsurrel added a reviewer: Frameworks. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. thsurrel requested review of this revision. REVISION SUMMARY When we are navigating in an archive, trying to use the navigator buttons to list the subdir of a parent folder of the archive was not working because the protocol would still be 'tar' instead of 'file'. TEST PLAN In Dolphin, click on a zip file. Then in the URL bar (breadcrumb mode), try to list the sub directories of the folder containing that zip file by clicking on the litlle arrow on the right of the folder's name. REPOSITORY R241 KIO BRANCH arc_urlnavigatorbutton (branched from master) REVISION DETAIL https://phabricator.kde.org/D17270 AFFECTED FILES src/filewidgets/kurlnavigatorbutton.cpp To: thsurrel, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
