Update formulas and add option to change grid thickness
Needs ReviewPublic

Authored by piotrkakol on Jul 7 2018, 10:27 PM.

Details

Reviewers
None
Group Reviewers
Plasma
Summary

Cleaned code for binary clock applet and added comments.

Changed refresh time from 30 to 60 seconds.

Changed variable type to int to display squares instead of rectangles.

Renamed dot to square in variables and filenames.

Fixed formulas for better handling border conditions.

Added option to change border thickness.

Diff Detail

Repository
R114 Plasma Addons
Branch
binary-clock-fixes (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 666
Build 678: arc lint + arc unit
piotrkakol created this revision.Jul 7 2018, 10:27 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 7 2018, 10:27 PM
piotrkakol requested review of this revision.Jul 7 2018, 10:27 PM


On the left there's the old applet and on the right mine after only changing dotSize (now squareSize) from real to int. All squares, not rectangles.


This is the config window after my changes.


This gif shows that after my changes the applet is scaling way better for smaller horizontal taskbar. You can notice that in the end lower part disappears below the screen. This only happens during recording the gif with Peek. Check for yourselves.


This gif shows that the new applet works as well for the vertical taskbar. Like with the above gif, the left part of the applet disappears but it's also only during recording with Peek.

I don't have KDE developer account so I cannot push this patch.

Wow, this is nice :) I wonder if it could be broken into smaller pieces though, this is quite a big change and thus hard to review. For example if the renaming from "dot" to "square" was independent of the rest, that would make it easier. Was it developed as individual changes? Then it should be relatively easy to split up.

piotrkakol added a comment.EditedJul 20 2018, 3:09 PM

Thanks. It can be breaked into smaller pieces. In fact, these 6 lines in the summary are titles of my commits that artisan combined into 1 diff.
This is why Phabricator is quite inconvenient. On GitHub you can have multiple commits in one Pull Request (diff equivalent), committer doesn't need to have merge rights, so a repo maintainer who has merges the PR.
E.g. here there are 2 commits in 1 PR.
Here I can't make commits - only diffs. And from what I understand 1 diff should be 1 commit. So do I really need to make 1 diff for the 1st commit, then wait for it to be merged, add 2nd diff, etc. up to 6th diff/commit? I could make 6 diffs now but some of them would have to include changes from other diffs. And I don't know if diffs can have parents.

Whould you mind sharing what would be the easiest way for me to handle this?

With Phabricator, patches can have parents, and multi-commit dependency chains are fully supported: https://community.kde.org/Infrastructure/Phabricator#If_the_patches_are_all_for_the_same_project

I agree that this particular workflow is a bit more cumbersome than it is with Github though. However with Phabricator other things are easier (no need to manage a local fork of each each repo you want to submit patches to, no need to use the web interface to submit patches).

Thanks for the link! I'll check it out probably tomorrow and will make new diff(s). :-)