Review Request: Weather Forecast QML port

[Luis Gabriel Lima]

> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/CMakeLists.txt, line 15
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81338#file81338line15>
> >
> >     Where do you use this?

Nowhere actually. It was there in the old version and I forgot to remove. Thanks.


> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/package/contents/ui/FiveDaysView.qml, line 65
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81340#file81340line65>
> >
> >     What is this doing? Changing the text from inside "color: " isn't very intuitive to read. 
> >     
> >     You must be mixing logic and styling which isn't ever advised for maintainable code.
> >     
> >     Needs a comment at least, ideally rewriting.

You're right, change the text from inside the color assigment code is a bit weird. It's fixed now.


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

There is no 'alpha' property in the Text element. Are you suggesting to use the 'opacity' property instead of a color with alpha channel?


> 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

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.


> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/weatherapplet.h, line 68
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81348#file81348line68>
> >
> >     Having a slot with the same name as a signal is really confusing. You end up emitting dataUpdated from dataUpdated.
> >

You're right. Fixed.


> 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()

What I need to do to have this plasmoid object in the qml context?


- Luis Gabriel


-----------------------------------------------------------
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/7471c871/attachment-0001.html>