D17464: [Timer applet] Minor fixes for the applet

gepardo added a comment. Thanks, now I got your point much clearer. REPOSITORY R114 Plasma Addons REVISION DETAIL https://phabricator.kde.org/D17464 To: gepardo, muhlenpfordt, mmazur, friedreich, #plasma, ngraham, davidedmundson Cc: ngraham, davidedmundson, plasma-devel, LeGast00n,

D17464: [Timer applet] Minor fixes for the applet

davidedmundson requested changes to this revision. davidedmundson added a comment. This revision now requires changes to proceed. If it's the exact same code as before, then my opinion will be the same. The digital clock set the text height to 0.7 of the panel and the width is inferred

D17464: [Timer applet] Minor fixes for the applet

gepardo added a comment. ping @davidedmundson REPOSITORY R114 Plasma Addons BRANCH arcpatch-D17464 REVISION DETAIL https://phabricator.kde.org/D17464 To: gepardo, muhlenpfordt, mmazur, friedreich, #plasma, ngraham Cc: ngraham, davidedmundson, plasma-devel, LeGast00n,

D17464: [Timer applet] Minor fixes for the applet

gepardo added a comment. Still waiting for the feedback. REPOSITORY R114 Plasma Addons BRANCH arcpatch-D17464 REVISION DETAIL https://phabricator.kde.org/D17464 To: gepardo, muhlenpfordt, mmazur, friedreich, #plasma, ngraham Cc: ngraham, davidedmundson, plasma-devel, LeGast00n,

D17464: [Timer applet] Minor fixes for the applet

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. LGTM, but let's wait for @davidedmundson. REPOSITORY R114 Plasma Addons BRANCH arcpatch-D17464 REVISION DETAIL https://phabricator.kde.org/D17464 To: gepardo, muhlenpfordt,

D17464: [Timer applet] Minor fixes for the applet

gepardo added a comment. Ping @davidedmundson REPOSITORY R114 Plasma Addons REVISION DETAIL https://phabricator.kde.org/D17464 To: gepardo, muhlenpfordt, mmazur, friedreich, #plasma Cc: ngraham, davidedmundson, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel,

D17464: [Timer applet] Minor fixes for the applet

gepardo added a comment. Here is how it looks on tall panels: F7114427: clock2.png REPOSITORY R114 Plasma Addons REVISION DETAIL https://phabricator.kde.org/D17464 To: gepardo, muhlenpfordt, mmazur, friedreich, #plasma Cc: ngraham,

D17464: [Timer applet] Minor fixes for the applet

gepardo updated this revision to Diff 62751. gepardo added a comment. Now the widget doesn't become too wide if the panel is tall. The size formula now looks similar to the size formula of the clock widget. Expanatory comment is updated. REPOSITORY R114 Plasma Addons CHANGES SINCE LAST

D17464: [Timer applet] Minor fixes for the applet

gepardo added a comment. > I don't follow how it makes them the same when the other number is different. Timer applet is rendered using SVGs, clock applet is rendered using text. So, as the SVGs and font glyphs have different sizes, we need different coefficients. The coefficient 0.86

D17464: [Timer applet] Minor fixes for the applet

gepardo updated this revision to Diff 62750. gepardo added a comment. Change magic value to 0.86, as it looks nearer to the clock font size REPOSITORY R114 Plasma Addons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17464?vs=62730=62750 BRANCH arcpatch-D17464 REVISION DETAIL

D17464: [Timer applet] Minor fixes for the applet

davidedmundson added a comment. > The rationale is to make the clock font size and the timer font size identical. Setting 0.84 as timer height multiplier indeed reaches this goal. But two other problems remain: I don't follow how it makes them the same when the other number is

D17464: [Timer applet] Minor fixes for the applet

gepardo updated this revision to Diff 62730. gepardo added a comment. Minor change in the comment for the "magic" value 0.84 REPOSITORY R114 Plasma Addons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17464?vs=62660=62730 BRANCH arcpatch-D17464 REVISION DETAIL

D17464: [Timer applet] Minor fixes for the applet

gepardo added a comment. The rationale is to make the clock font size and the timer font size identical. Setting 0.84 as timer height multiplier indeed reaches this goal. But two other problems remain: - the fonts are different itself, as clock uses the real font, and timer uses SVGs

D17464: [Timer applet] Minor fixes for the applet

davidedmundson added a comment. I still can't accept a patch with an unexplained magic number. If the rationale is that it's to match the clock, then it has to match the clock. If there's something else, please can the comment say what. REPOSITORY R114 Plasma Addons REVISION DETAIL

D17464: [Timer applet] Minor fixes for the applet

gepardo marked an inline comment as done. gepardo added a comment. After some experiments, I finally set clock size to 0.84: F7107969: clock.png The main problem is that the clock widget uses a real font, but the timer uses SVGs to render

D17464: [Timer applet] Minor fixes for the applet

gepardo updated this revision to Diff 62660. gepardo added a comment. Updating D17464 <https://phabricator.kde.org/D17464>: [Timer applet] Minor fixes for the applet Change clock size ratio to 0.84 for better compatibility with Clock's font size REPOSITORY R114 Plasma Addons C

D17464: [Timer applet] Minor fixes for the applet

davidedmundson added inline comments. INLINE COMMENTS > davidedmundson wrote in TimerView.qml:65 > I dislike the clock code, but if you leave a comment of why it's that way, > then ok. Clock is 0.71 REPOSITORY R114 Plasma Addons REVISION DETAIL https://phabricator.kde.org/D17464 To:

D17464: [Timer applet] Minor fixes for the applet

gepardo updated this revision to Diff 47224. gepardo added a comment. Add a comment about value 0.77 REPOSITORY R114 Plasma Addons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17464?vs=47219=47224 BRANCH timer-on-panel-fix (branched from master) REVISION DETAIL

D17464: [Timer applet] Minor fixes for the applet

gepardo added inline comments. INLINE COMMENTS > davidedmundson wrote in TimerView.qml:53 > never set size hints from current size, you're asking for binding loops. > > setting it based on the implicitWith is ok Can't get it work with `implicitWidth` and `implicitHeight`. By the way, when

D17464: [Timer applet] Minor fixes for the applet

davidedmundson added inline comments. INLINE COMMENTS > gepardo wrote in TimerView.qml:65 > As I remember, I was trying to make the digits size of timer equal nearer to > clock font size. Setting this to `1.0` won't break anything, but the digits > will take the full height of the panel. > >

D17464: [Timer applet] Minor fixes for the applet

gepardo added inline comments. INLINE COMMENTS > davidedmundson wrote in TimerView.qml:65 > where has 0.77 come from As I remember, I was trying to make the digits size of timer equal nearer to clock font size. Setting this to `1.0` won't break anything, but the digits will take the full

D17464: [Timer applet] Minor fixes for the applet

gepardo added inline comments. INLINE COMMENTS > davidedmundson wrote in TimerView.qml:46 > what if it's neither (i.e on the desktop) ? Doesn't it use the default sizing settings (as used before this patch)? REPOSITORY R114 Plasma Addons REVISION DETAIL https://phabricator.kde.org/D17464

D17464: [Timer applet] Minor fixes for the applet

davidedmundson added a comment. Cool. INLINE COMMENTS > TimerView.qml:46 > +states: [ > +State { > +name: "verticalPanel" what if it's neither (i.e on the desktop) ? > TimerView.qml:53 > +Layout.fillWidth: true > +

D17464: [Timer applet] Minor fixes for the applet

gepardo created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. gepardo requested review of this revision. REVISION SUMMARY BUG: 395182 BUG: 361025 Here are the changes made: - Allow Timer widget to work well on panels, both vertical and