[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-devel
Subject: Re: Several new utilities for KDE: KLook and StackFolder
From: Sergey Borovkov <serge.borovkov () gmail ! com>
Date: 2012-05-11 7:23:37
Message-ID: CACYfbRiea1-GcR-CX2kJejAENeiJ755iWFmgXeVWxbYdP8Kf7Q () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Hi,
On Mon, May 7, 2012 at 9:15 PM, David Edmundson
<david@davidedmundson.co.uk>wrote:
> Good to see these merged into playground!
>
> Some things we would need to proposing for KDE extragear/core
> - Watch your coding style
> http://techbase.kde.org/Policies/Kdelibs_Coding_Style
> In particular:
> Always use braces
> Don't call classes "MySomething". As the "My" isn't very helpful
> Make sure the files actually match the names of the class.
> audio.h, video.h, text.h
> MyAudio::~MyAudio() use m_mediaObject->deleteLater(); instead of
> delete (it's safer). Alternatively set a parent on it in the
> constructor and don't manually delete it.
> Technically your whitespace is off in many ways too, but that's
> being pedantic.
>
> - Long term rendering of audio/text/video/images etc in QML needs to
> be moved out of there and into somewhere more generic (some sort of
> generic QML component that can display any mimetype using the
> appropriate backends)
>
> - This has a lot of duplication with the previewer applet (which is
> why we need the above to fix it). This still needs a QML port (AFAIK),
> so it would be a good time to work together as it's basically the same
> thing. (Frankly yours is a lot better right now)
>
> - The way you're embedding video in QML isn't always going to work.
> (it won't work with xine using xv anyway, but I think that's being
> deprecated)..you don't have a lot of choice right now, but make sure
> you talk to someone from KDE Multimedia about what's moving forward
> with Phonon-QML.
>
> - Most importantly you've hardcoded it to match the theme you're
> using in your distro. However on any other setup (like stock KDE) your
> buttons look completely different from anything in my normal KDE
> applications, and completely different from anything in Plasma. Same
> for the window frame, and the icons etc. It looks absolutely great in
> your screenshots where it matches the theme you're using, but on any
> other setup it looks awful/out of place. Inside KDE we need to keep
> consistency.
>
> Not trying to be negative, it's a great start!
>
> David Edmundson
>
> >> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to
> unsubscribe <<
>
Thank you for feedback. We'll try to improve code on points you mentioned
in future. I see some problems though with your last suggestion. We
understand that hardcoding colors to match theme is not good. But,
unfortunately, when we used standard components they just don't look as
good. And design was one of our top priorities.
[Attachment #5 (text/html)]
<br>Hi, <br><div class="gmail_quote">On Mon, May 7, 2012 at 9:15 PM, David \
Edmundson <span dir="ltr"><<a href="mailto:david@davidedmundson.co.uk" \
target="_blank">david@davidedmundson.co.uk</a>></span> \
wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"> Good to see these merged \
into playground!<br> <br>
Some things we would need to proposing for KDE extragear/core<br>
- Watch your coding style<br>
<a href="http://techbase.kde.org/Policies/Kdelibs_Coding_Style" \
target="_blank">http://techbase.kde.org/Policies/Kdelibs_Coding_Style</a><br>
In particular:<br>
Always use braces<br>
Don't call classes "MySomething". As the "My" \
isn't very helpful<br> Make sure the files actually match the names of \
the class.<br> audio.h, video.h, text.h<br>
MyAudio::~MyAudio() use m_mediaObject->deleteLater(); instead \
of<br> delete (it's safer). Alternatively set a parent on it in the<br>
constructor and don't manually delete it.<br>
Technically your whitespace is off in many ways too, but that's<br>
being pedantic.<br>
<br>
- Long term rendering of audio/text/video/images etc in QML needs to<br>
be moved out of there and into somewhere more generic (some sort of<br>
generic QML component that can display any mimetype using the<br>
appropriate backends)<br>
<br>
- This has a lot of duplication with the previewer applet (which is<br>
why we need the above to fix it). This still needs a QML port (AFAIK),<br>
so it would be a good time to work together as it's basically the \
same<br> thing. (Frankly yours is a lot better right now)<br>
<br>
- The way you're embedding video in QML isn't always going to \
work.<br> (it won't work with xine using xv anyway, but I think \
that's being<br> deprecated)..you don't have a lot of choice right \
now, but make sure<br> you talk to someone from KDE Multimedia about \
what's moving forward<br> with Phonon-QML.<br>
<br>
- Most importantly you've hardcoded it to match the theme \
you're<br> using in your distro. However on any other setup (like stock \
KDE) your<br> buttons look completely different from anything in my normal \
KDE<br> applications, and completely different from anything in Plasma. \
Same<br> for the window frame, and the icons etc. It looks absolutely great \
in<br> your screenshots where it matches the theme you're using, but on \
any<br> other setup it looks awful/out of place. Inside KDE we need to \
keep<br> consistency.<br>
<br>
Not trying to be negative, it's a great start!<br>
<span class="HOEnZb"><font color="#888888"><br>
David Edmundson<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
>> Visit <a href="http://mail.kde.org/mailman/listinfo/kde-devel#unsub" \
target="_blank">http://mail.kde.org/mailman/listinfo/kde-devel#unsub</a> to \
unsubscribe <<<br></div></div></blockquote><div><br></div><div> Thank \
you for feedback. We'll try to improve code on points you mentioned in \
future. I see some problems though with your last suggestion. We understand \
that hardcoding colors to match theme is not good. But, unfortunately, when \
we used standard components they just don't look as good. And design \
was one of our top priorities. </div> </div><br>
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic