Review Request: Weather Forecast QML port

[David Edmundson]

> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/package/contents/ui/FiveDaysView.qml, line 70
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81340#file81340line70>
> >
> >     you don't need this.. just use 
> >     
> >     color: theColour
> >     alpha: theAlpha
> >     
> >     merging them into a string, just for QML to split it again seems odd.
> 
> Luis Gabriel Lima wrote:
>     There is no 'alpha' property in the Text element. Are you suggesting to use the 'opacity' property instead of a color with alpha channel?

I meant opacity, yeah. AFAIK they're the same thing.


> 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.

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.


> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/weatherapplet.h, line 69
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81348#file81348line69>
> >
> >     You don't need this.
> >     
> >     In your QML you can call plasmoid.openUrl()
> 
> Luis Gabriel Lima wrote:
>     What I need to do to have this plasmoid object in the qml context?

Normally in a QML plasmoid it's a global property. 

Thinking about it now, you've got QML being loaded inside a C++ applet, so that might not be the case. If not, just ignore this.


- David


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


On Aug. 26, 2012, 7:59 p.m., Luis Gabriel Lima wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106225/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2012, 7:59 p.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/20120829/ff30251b/attachment.html>