mmazur added inline comments. INLINE COMMENTS
> davidedmundson wrote in TimerView.qml:54 > that's a lot of wakeups. Do you really need to do this? Typical display is 60Hz, so 16.6 ms per frame. A 20ms interval means each clock tick will be displayed between 58-62 frames after the previous one, though usually 59-61. That's 966.8-1033.2ms delay between each tick. I'm pretty sure that's not noticeable to a human, even including any additional random delays caused by all the display logic. But If I increase it too much, it will become noticeable at some point. I guess I can change that to, say 25ms, while still being in the 58-62 range, but I really don't know what difference that 20Hz vs 25Hz makes on a 1GHz cpu? > davidedmundson wrote in TimerView.qml:56 > There's a behavioural change not mentioned in the commit message :/ > > If I have a timer for 5 minutes and suspend after 1 minute for an hour in > the old code when we resume we will see a timer for 4 minutes left. In the > new code we will see the timer has finished. > > You can make an argument this is better, but it shouldn't be accidental. > It's also not immune to timezone changes; we'd want this always in GMT. > > *if* we want this behaviour the clock dataengine might be a good solution, > that aligns itself to seconds automatically. > If we don't, maybe we want to neatly expose a QElapsedTimer; possibly > updating a property on every QQuickWindow::beforeSyncronisation. I wasn't aware that Date().getTime() wasn't in UTC. I can change it to a UTC call if there is one. I'm aware of the sleep behavior change, however I do not know how to handle it. (I was hoping the reviewer wouldn't notice.) If there's a trivial way to bind to signals for 'system is suspending'/'system just woke up', then I can pause the timer on those signals. What I cannot do is rewrite the code to use QSomethingOrOther, since I don't know Qt and I'm not going to learn Qt. Just so we're on the same page – nobody cared about this plasmoid for a few years now. To the extent that it plain stopped working when I upgraded to ubuntu 18.04 due to bug 381173. I fixed it the way I knew how and I'm happy with my code, since I can now use the plasmoid. If you have a perfect solution in mind, then you either need to code it yourself or find someone else who will. As far as interacting with me goes, it's either merge my code with minor improvements here and there (I can do the sleep handling if there's a signal for it) or don't and have the thing not work. If it's the former, please tell me upfront, so we don't waste each other's time on further discussions. REPOSITORY R114 Plasma Addons REVISION DETAIL https://phabricator.kde.org/D12536 To: mmazur, #plasma, davidedmundson Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
