[systemsettings] Ability to rename SD card and partitions. JB#54905 #3

amakarik · 2021-07-08T12:04:39Z

No description provided.

pvuorela · 2021-07-09T13:23:30Z

@Tomin1 could you check

src/udisks2monitor.cpp

rainemak · 2021-08-25T08:32:25Z

Ping @Tomin1

Tomin1

Left a few comments. I didn't try this yet.

src/partitionmodel.cpp

src/partitionmanager.cpp

src/udisks2monitor.cpp

Tomin1

Some error handling things and I started wondering about one UI design related thing, see the comments below.

src/udisks2monitor.cpp

amakarik · 2021-10-25T14:48:25Z

Now this has error handling in the sense that it does log error but it doesn't give UI any chance of showing that something > went wrong. It just stops. That doesn't seem good to me, the very least it should emit error signal. However if you didn't do > unmount / mount in setLabel, then you would not need to handle that many errors."

-fixed

pvuorela · 2021-11-01T12:09:01Z

src/partition.h

@@ -73,7 +73,8 @@ class SYSTEMSETTINGS_EXPORT Partition
        Unlocking,
        Unlocked,
        Locking,
-        Locked
+        Locked,
+        Renamed


Is this needed? The mounted/unmounted type of statuses make sense and so do transient mounting/unmounting etc, but this sort of of "end states" feel funny. The "Formatted" is an existing thing, but are they really different from "Unmounted" other than that the partition tracking has seen or initiated some related operation?

Done. Removed Renamed status

pvuorela · 2021-11-02T08:45:10Z

Generally looks fine for me now.

One thing missing from the API, though, is any kind of indication there is this task ongoing or when it succeeds. The Renamed status did the latter part but for the mentioned reasons it wasn't good either. a transient "Renaming" type of status could be ok. But how much that matters depends on the UI. Perhaps could be done separately too.

amakarik · 2021-11-02T09:25:33Z

Generally looks fine for me now.

One thing missing from the API, though, is any kind of indication there is this task ongoing or when it succeeds. The Renamed status did the latter part but for the mentioned reasons it wasn't good either. a transient "Renaming" type of status could be ok. But how much that matters depends on the UI. Perhaps could be done separately too.

Done. Add "Renaming" status

pvuorela · 2021-11-02T10:20:16Z

src/udisks2monitor.cpp

+        watcher->deleteLater();
+    });
+
+    emit status(devicePath, Partition::Renaming);


This is now the only new status change on the last update. Per se good, but what's now removing the Renaming state when the operation is done?

Ok, I see. What status should be set when operation is done? "Renamed"?

I'd expect Unmounted if the operation can be done only to unmounted partitions.

pvuorela · 2021-11-02T12:48:37Z

src/udisks2monitor.cpp

+    connect(watcher, &QDBusPendingCallWatcher::finished,
+            this, [this, devicePath, dbusMethod](QDBusPendingCallWatcher *watcher) {
+        if (watcher->isValid() && watcher->isFinished()) {
+            emit status(devicePath, Partition::Unmounted);


On error this stays forever on Renaming?

For small polish could move the status changes after the 'if', on both branches it's the last thing done.

pvuorela · 2021-11-03T11:40:11Z

One minor comment but besides that looks at least good to me. Could still test once the UI gets some polish.

pvuorela · 2021-11-16T12:42:32Z

Tried running this with the current UI side PR and renaming a vfat partition. Didn't succeed.

For the errors the journal didn't include much, just:
[W] unknown:0 - "SetLabel" error: org.freedesktop.UDisks2.Error.Failed

The UI didn't complain about anything.

With dbus-monitor on the system bus noticed:
string "Error setting label: Error spawning command-line `dosfslabel "/dev/mmcblk1p1" "SD-KORTTIHJ"': Failed to execute child process “dosfslabel” (No such file or directory) (g-exec-error-quark, 8)"

Would be included in dosfstools, but that's gplv3 and compiled into tools side only https://github.com/mer-tools/dosfstools

Instead we have busybox-symlinks-dosfstools which doesn't seem to include such a thing.

amakarik · 2021-11-26T07:49:41Z

Tried running this with the current UI side PR and renaming a vfat partition. Didn't succeed.

For the errors the journal didn't include much, just: [W] unknown:0 - "SetLabel" error: org.freedesktop.UDisks2.Error.Failed

The UI didn't complain about anything.

With dbus-monitor on the system bus noticed: string "Error setting label: Error spawning command-line `dosfslabel "/dev/mmcblk1p1" "SD-KORTTIHJ"': Failed to execute child process “dosfslabel” (No such file or directory) (g-exec-error-quark, 8)"

Would be included in dosfstools, but that's gplv3 and compiled into tools side only https://github.com/mer-tools/dosfstools

Instead we have busybox-symlinks-dosfstools which doesn't seem to include such a thing.

It requires dosfstools to be installed. It doesn't work with busybox-symlinks-dosfstools.

pvuorela requested a review from Tomin1 August 12, 2021 11:56

rainemak self-requested a review August 13, 2021 06:13

okodron reviewed Aug 24, 2021

View reviewed changes

src/udisks2monitor.cpp Outdated Show resolved Hide resolved

amakarik force-pushed the jb54905 branch from 685d695 to 552eb9f Compare August 25, 2021 13:09

Tomin1 reviewed Aug 25, 2021

View reviewed changes

src/partitionmodel.cpp Outdated Show resolved Hide resolved

src/partitionmanager.cpp Outdated Show resolved Hide resolved

src/udisks2monitor.cpp Outdated Show resolved Hide resolved

amakarik force-pushed the jb54905 branch from 552eb9f to c4bcb26 Compare August 26, 2021 08:04

Tomin1 reviewed Aug 26, 2021

View reviewed changes

src/udisks2monitor.cpp Outdated Show resolved Hide resolved

src/udisks2monitor.cpp Outdated Show resolved Hide resolved

amakarik force-pushed the jb54905 branch from c4bcb26 to 05ff239 Compare October 25, 2021 14:37

amakarik force-pushed the jb54905 branch from 05ff239 to 62f6284 Compare October 29, 2021 06:18

pvuorela reviewed Nov 1, 2021

View reviewed changes

amakarik force-pushed the jb54905 branch from 62f6284 to 929b6c4 Compare November 2, 2021 07:09

amakarik force-pushed the jb54905 branch from 929b6c4 to 57394ca Compare November 2, 2021 09:25

pvuorela reviewed Nov 2, 2021

View reviewed changes

amakarik force-pushed the jb54905 branch from 57394ca to 6baadf0 Compare November 2, 2021 11:31

pvuorela reviewed Nov 2, 2021

View reviewed changes

amakarik force-pushed the jb54905 branch from 6baadf0 to d2b685e Compare November 2, 2021 13:13

[systemsettings] Ability to rename SD card and partitions. JB#54905

bfd0866

amakarik force-pushed the jb54905 branch from d2b685e to bfd0866 Compare November 3, 2021 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[systemsettings] Ability to rename SD card and partitions. JB#54905 #3

[systemsettings] Ability to rename SD card and partitions. JB#54905 #3

amakarik commented Jul 8, 2021

pvuorela commented Jul 9, 2021

rainemak commented Aug 25, 2021

Tomin1 left a comment

Tomin1 left a comment

amakarik commented Oct 25, 2021

pvuorela Nov 1, 2021

amakarik Nov 2, 2021

pvuorela commented Nov 2, 2021

amakarik commented Nov 2, 2021

pvuorela Nov 2, 2021

amakarik Nov 2, 2021

pvuorela Nov 2, 2021

amakarik Nov 2, 2021

pvuorela Nov 2, 2021

amakarik Nov 2, 2021

pvuorela Nov 3, 2021

amakarik Nov 3, 2021

pvuorela commented Nov 3, 2021

pvuorela commented Nov 16, 2021

amakarik commented Nov 26, 2021

[systemsettings] Ability to rename SD card and partitions. JB#54905 #3

Are you sure you want to change the base?

[systemsettings] Ability to rename SD card and partitions. JB#54905 #3

Conversation

amakarik commented Jul 8, 2021

pvuorela commented Jul 9, 2021

rainemak commented Aug 25, 2021

Tomin1 left a comment

Choose a reason for hiding this comment

Tomin1 left a comment

Choose a reason for hiding this comment

amakarik commented Oct 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvuorela commented Nov 2, 2021

amakarik commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvuorela commented Nov 3, 2021

pvuorela commented Nov 16, 2021

amakarik commented Nov 26, 2021