D25991: [KFilePlacesModel] Fix supported scheme check for devices
This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit R241:3a6e3f927111: [KFilePlacesModel] Fix supported scheme check for devices (authored by broulik). Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D25991?vs=71522=72708#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25991?vs=71522=72708 REVISION DETAIL https://phabricator.kde.org/D25991 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp To: broulik, #frameworks, dfaure, ngraham, bruns Cc: kde-frameworks-devel, bruns, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25991: [KFilePlacesModel] Fix supported scheme check for devices
ngraham added a comment. No response, I think you can land this. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D25991 To: broulik, #frameworks, dfaure, ngraham, bruns Cc: bruns, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25991: [KFilePlacesModel] Fix supported scheme check for devices
broulik added a comment. Ping REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D25991 To: broulik, #frameworks, dfaure, ngraham, bruns Cc: bruns, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25991: [KFilePlacesModel] Fix supported scheme check for devices
ngraham added a comment. @bruns? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D25991 To: broulik, #frameworks, dfaure, ngraham, bruns Cc: bruns, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25991: [KFilePlacesModel] Fix supported scheme check for devices
broulik added inline comments. INLINE COMMENTS > bruns wrote in kfileplacesmodel.cpp:758 > you have dropped the `allowedHere` check. Probably just `if (!allowedHere) > continue`. > > And for reasons unknown to me, `tags:` is not filtered per app - move the > allowedHere check to the very beginning of the `while(...)` loop. Devices are always allowed. The old check was if (isSupportedScheme && ((isSupportedUrl && ... && allowedHere) || deviceAvailable)) so `deviceAvailable` trumps `allowedHere` > bruns wrote in kfileplacesmodel.cpp:759 > Does KFilePlacesItem have a move constructor? You can do the heap allocation > then only if necessary. It does not. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D25991 To: broulik, #frameworks, dfaure, ngraham, bruns Cc: bruns, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25991: [KFilePlacesModel] Fix supported scheme check for devices
bruns added inline comments.
INLINE COMMENTS
> kfileplacesmodel.cpp:758
> +KFilePlacesItem *item = nullptr;
> +if (deviceAvailable) {
> +item = new KFilePlacesItem(bookmarkManager,
> bookmark.address(), udi);
you have dropped the `allowedHere` check. Probably just `if (!allowedHere)
continue`.
And for reasons unknown to me, `tags:` is not filtered per app - move the
allowedHere check to the very beginning of the `while(...)` loop.
> kfileplacesmodel.cpp:759
> +if (deviceAvailable) {
> +item = new KFilePlacesItem(bookmarkManager,
> bookmark.address(), udi);
> +if (!item->hasSupportedScheme(supportedSchemes)) {
Does KFilePlacesItem have a move constructor? You can do the heap allocation
then only if necessary.
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D25991
To: broulik, #frameworks, dfaure, ngraham
Cc: bruns, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25991: [KFilePlacesModel] Fix supported scheme check for devices
ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. Oh so nice. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D25991 To: broulik, #frameworks, dfaure, ngraham Cc: ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D25991: [KFilePlacesModel] Fix supported scheme check for devices
broulik created this revision. broulik added reviewers: Frameworks, dfaure. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. broulik requested review of this revision. REVISION SUMMARY A device doesn't usually have a URL so we need to actutally check the underlying device (e.g. StorageAccess or NetworkShare) for whether it is supported. TEST PLAN - `kdialog --getopenfilename ~` now has my mounted ISOs and external storage now - didn't test with CD drives or network shares - KFilePlaces tests stil pass REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D25991 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp To: broulik, #frameworks, dfaure Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
