Review Request 122196: Offer the option to show per-field binary instead of per-digit

[Patrick Uiterwijk]

> On Jan. 22, 2015, 11:07 a.m., Sebastian Kügler wrote:
> > Code-wise, it has a few issues with the config saving and restoration, those are easily fixed, however.
> > 
> > Otherwise, the problem is that we've frozen Plasma 4.x for more than a year already, so we won't accept new features. All new feature development will have to be done against Plasma 5.x (in kdeplasma-addons, that would be the "frameworks" branch). I don't know about the porting status of the binary clock, but it'll need some adjustments in order to make it work, porting the UI to QML, for example. Best would be to port your patch to the Plasma 5.x version, and given the resulting code's quality is OK, I'd be quite fine with merging that.

Thanks for the feedback.
I checked the other branches, but there's no "frameworks" branch, just a "frameworks-scratch" branch that hasn't been touched since 2013.
There is however a Plasma/5.2 branch, which sounds like the right place, but it seems binary clock has not yet been ported to QML, and as such my patch directly applies there.

I would be willing to learn QML and port binary-clock to it, though I would wonder if it would be okay to merge this patch before so I can port it as part of the rest?


> On Jan. 22, 2015, 11:07 a.m., Sebastian Kügler wrote:
> > applets/binary-clock/binaryclock.cpp, line 166
> > <https://git.reviewboard.kde.org/r/122196/diff/1/?file=344087#file344087line166>
> >
> >     You probably want to read the value of perFieldBinary and initialize the checkbox with it. Otherwise it'll always default to false, no matter the actual setting.

Isn't this what line 221 should take care of?
m_usePerFieldBinary = cg.readEntry("perFieldBinary", m_usePerFieldBinary);


- Patrick


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122196/#review74528
-----------------------------------------------------------


On Jan. 22, 2015, 12:53 p.m., Patrick Uiterwijk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122196/
> -----------------------------------------------------------
> 
> (Updated Jan. 22, 2015, 12:53 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> -------
> 
> Offer the option to show per-field binary instead of per-digit
> 
> 
> Diffs
> -----
> 
>   applets/binary-clock/binaryclock.h 5ae83dfff9c473bd484b99d49330093b6e091975 
>   applets/binary-clock/binaryclock.cpp 75c4548588ff15e2316dd1c2e888ad8c07f7f78b 
>   applets/binary-clock/clockConfig.ui 0c2cec7925dd9e1ff92d7cd9d99005dca20497b0 
> 
> Diff: https://git.reviewboard.kde.org/r/122196/diff/
> 
> 
> Testing
> -------
> 
> I have compiled a patched version of kdeplasma-addons on a recent version of Fedora (22), and am using it myself.
> 
> 
> Thanks,
> 
> Patrick Uiterwijk
> 
>

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