Differential D8348

Add a section for removable devices
ClosedPublic
Actions

Authored by renatoo on Oct 17 2017, 7:20 PM.

Details

Reviewers
ervin
ngraham
mwolff
Group Reviewers
Dolphin
Frameworks
VDG
Commits
R241:3db04cfc5179: Add a section for removable devices
Summary

Show removable devices in a different section

Depends on D8243
Depends on D8332

Test Plan

From any kde app try to open/save a file.
Plug any removable device in your computer, make sure that it appears over 'Removable devices' section on places panel

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham added a comment.Oct 17 2017, 9:02 PM

Can you put the screenshot in the Summary section above, and list the dependencies there too? Also put dependency information there, in the following form:


Depends on D8243
Depends on D8332

This special syntax marks them as dependencies.

broulik added a reviewer: VDG.Oct 17 2017, 9:06 PM
renatoo added a comment.Oct 17 2017, 9:09 PM
In D8348#156659, @ngraham wrote:

Can you put the screenshot in the Summary section above, and list the dependencies there too? Also put dependency information there, in the following form:


Depends on D8243
Depends on D8332

This special syntax marks them as dependencies.

Done, Thanks for the information.

renatoo updated this revision to Diff 20941.Oct 18 2017, 11:45 AM
renatoo edited the summary of this revision. (Show Details)

Updated parent branch

renatoo updated this revision to Diff 20953.Oct 18 2017, 3:42 PM

Update parent branch

anthonyfieroni added a subscriber: anthonyfieroni.Oct 18 2017, 5:02 PM
anthonyfieroni added inline comments.
src/filewidgets/kfileplacesitem.cpp
90

Braces even on one line block.

329

Early exit to not add unwanted indent.

if (m_device.udi() == udi) {
    return false;
}
`
renatoo updated this revision to Diff 20987.Oct 19 2017, 7:11 PM
renatoo marked 2 inline comments as done.

Fixed code style

renatoo updated this revision to Diff 21003.Oct 20 2017, 12:15 PM

Fixed unit test

renatoo updated this revision to Diff 21228.Oct 24 2017, 11:11 AM

Updated parent branch

mlaurent added a subscriber: mlaurent.Oct 25 2017, 7:17 AM
mlaurent added inline comments.
src/filewidgets/kfileplacesitem.cpp
354

could you use new connect api please ?

renatoo updated this revision to Diff 21296.Oct 25 2017, 11:23 AM

Used new connect API

renatoo marked an inline comment as done.Oct 25 2017, 11:24 AM
renatoo updated this revision to Diff 21584.Oct 30 2017, 6:33 PM

Updated parent branch

ervin accepted this revision.Oct 31 2017, 11:25 AM
This revision is now accepted and ready to land.Oct 31 2017, 11:25 AM
ervin requested changes to this revision.Oct 31 2017, 11:32 AM

Found a small one after all.

src/filewidgets/kfileplacesitem_p.h
51

RemovableDevicesType (since you used the plural on the one above).

This revision now requires changes to proceed.Oct 31 2017, 11:32 AM
renatoo updated this revision to Diff 21622.Oct 31 2017, 11:37 AM

Use plural form for RemovableDevicesType enum

renatoo marked an inline comment as done.Oct 31 2017, 11:38 AM
ervin accepted this revision.Oct 31 2017, 11:47 AM
This revision is now accepted and ready to land.Oct 31 2017, 11:47 AM
renatoo updated this revision to Diff 21648.Oct 31 2017, 3:27 PM

Update parent branch

renatoo updated this revision to Diff 21759.Nov 2 2017, 11:44 AM

Updated parent branch

ngraham requested changes to this revision.Nov 2 2017, 3:10 PM

Could you add camera:/ devices to this section too? That way we could also take care of https://bugs.kde.org/show_bug.cgi?id=386060

This revision now requires changes to proceed.Nov 2 2017, 3:10 PM
renatoo updated this revision to Diff 21808.Nov 2 2017, 8:35 PM

Updated parent branch

renatoo updated this revision to Diff 21933.Nov 5 2017, 9:49 PM

Parent branch update

abetts added a subscriber: abetts.Nov 5 2017, 9:58 PM

What about something like this as well

The removable device gets an "unmount" or "disconnect" icon on hover?

Thoughts?

ngraham added a comment.Nov 5 2017, 10:12 PM
In D8348#164713, @abetts wrote:

What about something like this as well

The removable device gets an "unmount" or "disconnect" icon on hover?

Thoughts?

Yes yes yes! That would also resolve https://bugs.kde.org/show_bug.cgi?id=154499

Of course, if this isn't an easy change, we may not want to overload the patch.

renatoo updated this revision to Diff 22329.Nov 14 2017, 11:41 AM

Updated parent branch

mwolff added a subscriber: mwolff.Nov 15 2017, 10:19 AM
In D8348#164718, @ngraham wrote:
In D8348#164713, @abetts wrote:

What about something like this as well

The removable device gets an "unmount" or "disconnect" icon on hover?

Thoughts?

Yes yes yes! That would also resolve https://bugs.kde.org/show_bug.cgi?id=154499

Of course, if this isn't an easy change, we may not want to overload the patch.

This should be done in a follow-up patch

mwolff added a comment.Nov 15 2017, 10:19 AM
In D8348#163546, @ngraham wrote:

Could you add camera:/ devices to this section too? That way we could also take care of https://bugs.kde.org/show_bug.cgi?id=386060

this isn't done yet, I think

mwolff requested changes to this revision.Nov 15 2017, 10:34 AM

the UDI comment needs to be investigated, otherwise lgtm

autotests/kfileplacesmodeltest.cpp
180–181 ↗(On Diff #21296)

the changed list of remote urls that is repeated below could be put into another helper function

src/filewidgets/kfileplacesitem.cpp
91 ↗(On Diff #21296)

shouldn't this always be called? i.e. when the bookmark is changed to a different UDI, don't we need to update here, even when we had a valid device UDI before? I think this also means this path isn't unit tested yet

This revision now requires changes to proceed.Nov 15 2017, 10:34 AM
mwolff added inline comments.Nov 15 2017, 10:36 AM
autotests/kfileplacesmodeltest.cpp
180–181 ↗(On Diff #21296)

done already in https://phabricator.kde.org/D8366 I see, ignore this

renatoo added a comment.Nov 15 2017, 11:21 AM
In D8348#167924, @mwolff wrote:
In D8348#163546, @ngraham wrote:

Could you add camera:/ devices to this section too? That way we could also take care of https://bugs.kde.org/show_bug.cgi?id=386060

this isn't done yet, I think

This is not related with the change. I am working on that in a different branch.

renatoo marked 2 inline comments as done.Nov 15 2017, 11:22 AM
renatoo updated this revision to Diff 22393.Nov 15 2017, 1:40 PM

Update device info if udi changes.

renatoo marked an inline comment as done.Nov 15 2017, 1:40 PM
renatoo added inline comments.Nov 15 2017, 1:42 PM
src/filewidgets/kfileplacesitem.cpp
91 ↗(On Diff #21296)

Changed the code to always check for udi changes.
I tried to implement a unit test for this case, but I could not find a way to simulate it, and manual updating the bookmark udi cases the model to create a new item since uid is used as id for the model items.

ngraham accepted this revision.Nov 15 2017, 1:54 PM
In D8348#167982, @renatoo wrote:

This is not related with the change. I am working on that in a different branch.

OK, thanks! Please add me to the review list for that patch, once it's up.

mwolff accepted this revision.Nov 15 2017, 2:37 PM

lgtm too

This revision is now accepted and ready to land.Nov 15 2017, 2:37 PM
renatoo updated this revision to Diff 22453.Nov 16 2017, 1:39 PM

Updated parent branch

renatoo updated this revision to Diff 22764.Nov 22 2017, 5:57 PM

Updated parent branch

renatoo updated this revision to Diff 22776.Nov 22 2017, 7:15 PM

Updated from master

renatoo updated this revision to Diff 22811.Nov 23 2017, 11:53 AM

Parent branch updated

ngraham added a comment.Nov 26 2017, 3:57 AM

@renatoo, now that D8332 is in, this no patch longer applies cleanly. Can you update so I can land it?

renatoo updated this revision to Diff 22949.Nov 26 2017, 1:23 PM

Updated parent branch

renatoo added a comment.Nov 26 2017, 1:28 PM
In D8348#172032, @ngraham wrote:

@renatoo, now that D8332 is in, this no patch longer applies cleanly. Can you update so I can land it?

I updated it. Could you try again.

Closed by commit R241:3db04cfc5179: Add a section for removable devices (authored by Renato Araujo Oliveira Filho <renato.araujo@kdab.com>, committed by ngraham). · Explain WhyNov 26 2017, 2:44 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 21584

View Options

autotests/kfileplacesmodeltest.cpp

Loading...
View Options

src/filewidgets/kfileplacesitem.cpp

Loading...
View Options

src/filewidgets/kfileplacesitem_p.h

Loading...