Review Request: Weather Forecast QML port

[David Edmundson]

> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/package/contents/ui/Notice.qml, line 27
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81341#file81341line27>
> >
> >     Unless there's a good reason, use PlasmaComponents.Label rather than Text. Otherwise font size won't be consistent.
> >     
> >     Saves you setting the theme colour yourself too :)
> >     
> >     Same for all instances of Text
> 
> Luis Gabriel Lima wrote:
>     The C++ version uses the QApplication::font() instead of the plasma theme font. So after notice the difference when I was trying the PlasmaComponents.Label, I decided to keep the same way as the C++ version.
> 
> David Edmundson wrote:
>     That's sort of a good reason.
>     
>     Though I would argue that if the C++ one is wrong, there's no need to copy it.
> 
> Luis Gabriel Lima wrote:
>     I don't know if the C++ version is wrong, actually all the three plasmoid that I've ported was using QApplication::font().

It's still (IMHO) rather wrong. For example if a user had bad eyesight and increases their plasma font size in the Workspace Appearance KCM, you'd expect it to actually work. 

However, as it's not a regression so I'm not calling it a showstopper to merging.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106225/#review18054
-----------------------------------------------------------


On Aug. 30, 2012, 1:53 a.m., Luis Gabriel Lima wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106225/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2012, 1:53 a.m.)
> 
> 
> Review request for Plasma and Marco Martin.
> 
> 
> Description
> -------
> 
> This patch is part of the work done in my GSoC 2012 project.
> 
> 
> Diffs
> -----
> 
>   applets/weather/CMakeLists.txt 4ae9c6f 
>   applets/weather/package/contents/ui/DetailsView.qml PRE-CREATION 
>   applets/weather/package/contents/ui/FiveDaysView.qml PRE-CREATION 
>   applets/weather/package/contents/ui/Notice.qml PRE-CREATION 
>   applets/weather/package/contents/ui/NoticesView.qml PRE-CREATION 
>   applets/weather/package/contents/ui/TopPanel.qml PRE-CREATION 
>   applets/weather/package/contents/ui/Utils.js PRE-CREATION 
>   applets/weather/package/contents/ui/WeatherListView.qml PRE-CREATION 
>   applets/weather/package/contents/ui/main.qml PRE-CREATION 
>   applets/weather/package/metadata.desktop PRE-CREATION 
>   applets/weather/weatherapplet.h a7e7f8d 
>   applets/weather/weatherapplet.cpp 9fd3e5f 
>   applets/weather/weatherdelegate.h 8a27f0d 
>   applets/weather/weatherdelegate.cpp 1cd987b 
>   applets/weather/weatherview.h f177501 
>   applets/weather/weatherview.cpp 5234cd3 
> 
> Diff: http://git.reviewboard.kde.org/r/106225/diff/
> 
> 
> Testing
> -------
> 
> - Tested inside panels and floating on desktop
> - Tested with various providers and locations
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/106225/s/696/
> 
>   http://git.reviewboard.kde.org/r/106225/s/698/
> 
>   http://git.reviewboard.kde.org/r/106225/s/699/
> 
>   http://git.reviewboard.kde.org/r/106225/s/700/
> 
> 
> Thanks,
> 
> Luis Gabriel Lima
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120907/620f33fc/attachment.html>