[prev in list] [next in list] [prev in thread] [next in thread]
List: amarok-devel
Subject: Re: Review Request 111991: Better handling of non-writeable UMS collections
From: "Frank Meerkoetter" <frank () meerkoetter ! org>
Date: 2013-08-11 8:56:10
Message-ID: 20130811085610.14264.63567 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Aug. 10, 2013, 7:26 p.m., Mark Kretschmann wrote:
> > I'm not so sure about the warning that is shown when an unwritable UMS collection \
> > is loaded. First of all it seems to me this would belong on a higher level, as \
> > other collections could be read-only as well. Second, I'm not sure a warning is \
> > called for. It's not a critical situation. And last, doesn't it suffice to show \
> > an error when the actual writing failed?
>
> Frank Meerkoetter wrote:
> There won't be an error message when the actual writing fails, as the user is not \
> offered certain operations (move/copy to/organize) for read only collections. :-)
> I am not sure what would be a good solution. I have to admit that I added the \
> warning as an afterthought after having fixed the isWriteable() for UMS. The reason \
> was that i found it highly intransparent why certain operations are not longer \
> offered.
> The question is how to make it transparent to the user why move/copy/org. are \
> disabled for a certain UMS collection?
> Perhaps the solution is to leave isWriteable() as it was (always return true for \
> UMS) and to generate error messages when operations fail?
>
>
> Konrad Zemek wrote:
> I think that leaving isWritable() that returns true despite the fact that \
> collection is read-only is hacky at best, and dead wrong at worst. If we ever find \
> ourselves needing to do something like that, we have a sure sign of a faulty \
> design.
> That being said, I completely agree with Mark about it belonging on the higher \
> level. It may be nice to get a visual hint that a collection is read-only, but it \
> should do so for every collection type, not only for UMS.
> FWIW, I would instantly grasp the concept if the relevant actions were added but \
> disabled (as currently not being added at all). Maybe add a tooltip on hover saying \
> that the collection is read-only, and I'm sure everything would be clear enough. \
> Just something to consider, I'm not necessarily convinced it's the best way.
I like the idea of having the respective actions disabled (+ a tooltip explaining \
why). If there is a consensus about doing it like that i would implement it.
- Frank
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111991/#review37483
-----------------------------------------------------------
On Aug. 10, 2013, 4:44 p.m., Frank Meerkoetter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111991/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2013, 4:44 p.m.)
>
>
> Review request for Amarok.
>
>
> Description
> -------
>
> If an UMS-collection is non-writeable (think freshly created ext4 on an USB-stick) \
> a copy/move to collection will silently fail. It can only be seen in the debug logs \
> on the console.
> The reason is that the UMS-collection is not actually checking if its writeable. \
> Its just assuming to be always writeable.
>
> My change consists of two parts:
> 1. I check if the UMS-collection is actually writeable (mount point and if set the \
> "music folder"). 2. To give the user a hint what is going on I added a warning \
> message, informing the user that this collection is read-only (otherwise he might \
> be confused why certain operations (move/copy/organize) are missing.
>
>
> Diffs
> -----
>
> src/core-impl/collections/umscollection/UmsCollection.h 749ff81
> src/core-impl/collections/umscollection/UmsCollection.cpp 028966e
> src/core-impl/collections/umscollection/UmsCollectionLocation.cpp 1cd1ba8
>
> Diff: http://git.reviewboard.kde.org/r/111991/diff/
>
>
> Testing
> -------
>
> I have tested with an USB-Stick containing an ext4 filesystem. First i had the \
> base-dir owned by root.root (non-writeable), next i made it writeable and verified \
> that its correctly detected as such. Next i created a "Music" folder and configured \
> it for this USB-Stick. I checked that it was detected as writeable. Next i made the \
> "Music" folder non-writeable and checked that amarok also detected that.
>
> Thanks,
>
> Frank Meerkoetter
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/111991/">http://git.reviewboard.kde.org/r/111991/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On August 10th, 2013, 7:26 p.m. UTC, <b>Mark \
Kretschmann</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px \
solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">I'm not so sure about the warning that is shown when an unwritable \
UMS collection is loaded. First of all it seems to me this would belong on a higher \
level, as other collections could be read-only as well. Second, I'm not sure a \
warning is called for. It's not a critical situation. And last, doesn't it \
suffice to show an error when the actual writing failed?</pre> </blockquote>
<p>On August 10th, 2013, 8:32 p.m. UTC, <b>Frank Meerkoetter</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There won't be an \
error message when the actual writing fails, as the user is not offered certain \
operations (move/copy to/organize) for read only collections. :-)
I am not sure what would be a good solution. I have to admit that I added the warning \
as an afterthought after having fixed the isWriteable() for UMS. The reason was that \
i found it highly intransparent why certain operations are not longer offered.
The question is how to make it transparent to the user why move/copy/org. are \
disabled for a certain UMS collection?
Perhaps the solution is to leave isWriteable() as it was (always return true for UMS) \
and to generate error messages when operations fail?
</pre>
</blockquote>
<p>On August 10th, 2013, 8:44 p.m. UTC, <b>Konrad Zemek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think that leaving \
isWritable() that returns true despite the fact that collection is read-only is hacky \
at best, and dead wrong at worst. If we ever find ourselves needing to do something \
like that, we have a sure sign of a faulty design.
That being said, I completely agree with Mark about it belonging on the higher level. \
It may be nice to get a visual hint that a collection is read-only, but it should do \
so for every collection type, not only for UMS.
FWIW, I would instantly grasp the concept if the relevant actions were added but \
disabled (as currently not being added at all). Maybe add a tooltip on hover saying \
that the collection is read-only, and I'm sure everything would be clear enough. \
Just something to consider, I'm not necessarily convinced it's the best \
way.</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I like the idea of \
having the respective actions disabled (+ a tooltip explaining why). If there is a \
consensus about doing it like that i would implement it.
</pre>
<br />
<p>- Frank</p>
<br />
<p>On August 10th, 2013, 4:44 p.m. UTC, Frank Meerkoetter wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Amarok.</div>
<div>By Frank Meerkoetter.</div>
<p style="color: grey;"><i>Updated Aug. 10, 2013, 4:44 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">If an UMS-collection is non-writeable (think freshly created ext4 on an \
USB-stick) a copy/move to collection will silently fail. It can only be seen in the \
debug logs on the console.
The reason is that the UMS-collection is not actually checking if its writeable. Its \
just assuming to be always writeable.
My change consists of two parts:
1. I check if the UMS-collection is actually writeable (mount point and if set the \
"music folder"). 2. To give the user a hint what is going on I added a \
warning message, informing the user that this collection is read-only (otherwise he \
might be confused why certain operations (move/copy/organize) are missing.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">I have tested with an USB-Stick containing an ext4 filesystem. First i \
had the base-dir owned by root.root (non-writeable), next i made it writeable and \
verified that its correctly detected as such. Next i created a "Music" \
folder and configured it for this USB-Stick. I checked that it was detected as \
writeable. Next i made the "Music" folder non-writeable and checked that \
amarok also detected that.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/core-impl/collections/umscollection/UmsCollection.h <span style="color: \
grey">(749ff81)</span></li>
<li>src/core-impl/collections/umscollection/UmsCollection.cpp <span style="color: \
grey">(028966e)</span></li>
<li>src/core-impl/collections/umscollection/UmsCollectionLocation.cpp <span \
style="color: grey">(1cd1ba8)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111991/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic