[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: [KDEREVIEW] DeviceNotifier refactor
From: Alessandro Diaferia <alediaferia () gmail ! com>
Date: 2009-02-03 19:48:55
Message-ID: 65627f3a0902031148x75e5df66g8f01cbbacb18b29c () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
2009/2/3 Aaron J. Seigo <aseigo@kde.org>
> On Tuesday 03 February 2009, Alessandro Diaferia wrote:
> > I've just moved my DeviceNotifier refactoring to kdereview. I feel it is
> > much nicer but since i removed tons of code i'd like you to review it and
> > tell me if something is wrong.
>
> some comments/thoughts on the code (besides that Kevin already noted):
>
> * there's an m_rootItem in NotifierDialog that isn't used; looks like a
> hold-
> over from the current DeviceNotifier. ditto with the SpecificRoles enum.
yop, i forgot to remove them :P
>
>
> * NotifierDialog isn't actually a dialog anymore ;) perhaps a better name
> would make it clearer as to what it does. DeviceWidgetManager? looking at
> it a
> bit more, i wonder if NotifierDialog is even needed. all the real
> functionality is in the Applet class and DeviceWidget; the "in between"
> class
> could probably be removed as a middle-man and Applet could just use
> DeviceWidget directly?
i was thinking about the same.. i feel i'll remove the NotifierDialog :)
>
>
> * if setEjectEmblem is ever called with set == false and m_unmountActions
> doesn't contain an entry for that udi, a crash will occur. a Q_ASSERT
> should
> go in there at a minimum, if not a full if (m_unmountActions.contains).
> right
> now that should never be a problem, but it's one of those things that's
> really
> easy to introduce as a problem later on :)
I'm not sure i can imagine a case when set == false and m_unmountActions
does not contain the entry.
but i think i'll put the Q_ASSERT as you suggested :P
>
>
> looking at the screenshots i have two little comments:
>
> * the action seems to be on the "wrong" side of the dialog. reading
> left-to-
> right it should be "noun, verb" as in "the object i want to operate on, the
> thing i want to do with it" since before deciding to eject i first need to
> find what to eject ...
do you mean i should put the icon action on the right? will do :)
>
>
> looking at the code it seems that this is using Plasma::IconWidget to paint
> it; that icon should actually be in the corner of the icon itself, not the
> corner of the widget. so, that's a small bug in Plasma::IconWidget.
i will try to look at in and fix :P
>
>
> * the 128 px minimum size is probably a bit much. for preferred size it
> makes
> sense, but it should be possible to make them smaller than that.
right, just was waiting for suggestions =)
>
>
> otherwise, it's looking nice.. :)
thanks =)
>
>
> --
> Aaron J. Seigo
> humru othro a kohnu se
> GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43
>
> KDE core developer sponsored by Qt Software
>
>
> _______________________________________________
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>
>
Cheers
--
Alessandro Diaferia
[Attachment #5 (text/html)]
<br><br><div class="gmail_quote">2009/2/3 Aaron J. Seigo <span dir="ltr"><<a \
href="mailto:aseigo@kde.org">aseigo@kde.org</a>></span><br><blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt \
0pt 0.8ex; padding-left: 1ex;"> <div class="Ih2E3d">On Tuesday 03 February 2009, \
Alessandro Diaferia wrote:<br> > I've just moved my DeviceNotifier refactoring \
to kdereview. I feel it is<br> > much nicer but since i removed tons of code \
i'd like you to review it and<br> > tell me if something is wrong.<br>
<br>
</div>some comments/thoughts on the code (besides that Kevin already noted):<br>
<br>
* there's an m_rootItem in NotifierDialog that isn't used; looks like a \
hold-<br> over from the current DeviceNotifier. ditto with the SpecificRoles \
enum.</blockquote><div><br> yop, i forgot to remove them :P \
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, \
204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <br>
<br>
* NotifierDialog isn't actually a dialog anymore ;) perhaps a better name<br>
would make it clearer as to what it does. DeviceWidgetManager? looking at it a<br>
bit more, i wonder if NotifierDialog is even needed. all the real<br>
functionality is in the Applet class and DeviceWidget; the "in between" \
class<br> could probably be removed as a middle-man and Applet could just use<br>
DeviceWidget directly?</blockquote><div><br> i was thinking about the same.. i \
feel i'll remove the NotifierDialog :)<br> <br></div><blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt \
0pt 0.8ex; padding-left: 1ex;"> <br>
<br>
* if setEjectEmblem is ever called with set == false and m_unmountActions<br>
doesn't contain an entry for that udi, a crash will occur. a Q_ASSERT should<br>
go in there at a minimum, if not a full if (m_unmountActions.contains). right<br>
now that should never be a problem, but it's one of those things that's \
really<br> easy to introduce as a problem later on :)</blockquote><div><br> \
I'm not sure i can imagine a case when set == false and m_unmountActions does not \
contain the entry.<br> but i think i'll put the Q_ASSERT as you suggested \
:P<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, \
204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br> <br>
looking at the screenshots i have two little comments:<br>
<br>
* the action seems to be on the "wrong" side of the dialog. reading \
left-to-<br> right it should be "noun, verb" as in "the object i want \
to operate on, the<br> thing i want to do with it" since before deciding to \
eject i first need to<br> find what to eject ...</blockquote><div><br> do \
you mean i should put the icon action on the right? will do :) <br></div><blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt \
0pt 0.8ex; padding-left: 1ex;"> <br>
<br>
looking at the code it seems that this is using Plasma::IconWidget to paint<br>
it; that icon should actually be in the corner of the icon itself, not the<br>
corner of the widget. so, that's a small bug in \
Plasma::IconWidget.</blockquote><div><br> i will try to look at in and \
fix :P <br></div><blockquote class="gmail_quote" style="border-left: 1px solid \
rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <br>
<br>
* the 128 px minimum size is probably a bit much. for preferred size it makes<br>
sense, but it should be possible to make them smaller than \
that.</blockquote><div><br> right, just was waiting for suggestions \
=)<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, \
204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <br>
<br>
otherwise, it's looking nice.. :)</blockquote><div><br> thanks =) \
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, \
204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br> <font color="#888888"><br>
--<br>
Aaron J. Seigo<br>
humru othro a kohnu se<br>
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43<br>
<br>
KDE core developer sponsored by Qt Software<br>
<br>
</font><br>_______________________________________________<br>
Plasma-devel mailing list<br>
<a href="mailto:Plasma-devel@kde.org">Plasma-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/plasma-devel" \
target="_blank">https://mail.kde.org/mailman/listinfo/plasma-devel</a><br> \
<br></blockquote></div><br><br clear="all">Cheers<br>-- <br>Alessandro Diaferia<br>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic