Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add watchface analog-brawnish #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

David-Rinehart
Copy link

Add new watchface for analog-brawnish

Copy link
Member

@eLtMosen eLtMosen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like you mentioned on matrix, the README.md is missing your entry.
Thank you very much for your contribution!


Item {

// background
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd advise to use the id: property in a descriptive manner instead of comments as often as possible. Most convenient when using id: background is being able to reference it. That way other Items can for instance anchor to or reference local variables within the background Item. Even if you don't plan to use such in this project, mind that others might extend on your work. And we want to make it as convenient as possible later contributors.

property var hours: wallClock.time.getHours()

visible: !displayAmbient
width: 800
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded width/height might lead to different representation on different screen sizes. Tho we have no resolutions larger than 800px, this settings will extend the background over the watchface area. Which is visible when swiping in any direction from the watchface area. To fill the given screen completely without overlapping, we can use anchors.fill: parent

color: ((hours > 6 && hours < 18) ? "#eeeeee" : "#343334")
}

// os and slogan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be id: osSlogan put into the Text object two rows further.
Just since i did camelCase in this example -> camelCase is good practice in Qt/qml

Rectangle {
property var hours: wallClock.time.getHours()
property var rotM: (index - 15) /60
property var centerX: parent.width/2-width/2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistency in operator spacing. Most of the time you use the operators with spacings left and right -, *, etc. For a code reading beginner like me it is much easier to identify those operators when spaced.
It would greatly help me to make this consistent over the whole code.

Repeater {
model: 60
Rectangle {
property var hours: wallClock.time.getHours()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code duplication can be prevented here and in all other following Objects where you declare hour, minute and seconds individually.
When declared once in the root Item, all children can access them just the way they do do now. But saving declaration lines in the children objects.

property var centerY: parent.height/2-height/2

antialiasing : true
radius: 10.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent problem here, likely from Tab and Space mixup.


visible: !displayAmbient
antialiasing : true
radius: 2.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent problem like above

height: parent.width * 0.05
color: (hours > 6 && hours < 18) ? "whitesmoke" : "black"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary empty line

@@ -0,0 +1,259 @@
/*
* Copyright (C) 2022 - David Rinehart wdaver(at)cox.net
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RotM and CenterX/Y variables later on sound rather familiar. If you built this watchface using an example with given credits, it would be nice to the previous contributors to keep those mentions according to the license and extend your credits on top.
Don't take this as an offense please, if you just used the variable names but consider the code your sole work, there is nothing to complain ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants