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

Modification by SG #184

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

Modification by SG #184

wants to merge 1 commit into from

Conversation

SylvainGa
Copy link

Addition of French localization
Addition of Pulse Ox as a data field
Modification to the AlwaysOn to reduce batterie drain

Addition of French localization
Addition of Pulse Ox as a data field
Modification to the AlwaysOn to reduce batterie drain
@warmsound warmsound self-assigned this Jan 4, 2021
@warmsound
Copy link
Owner

@SylvainGa thanks for all your hard work on this 🤝 . I'm starting the review, and will submit comments shortly.

Copy link
Owner

@warmsound warmsound left a comment

Choose a reason for hiding this comment

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

Do you have a vector version of the O2 icon? If so, I'd like to update the crystal-icons.afdesign Affinity Designer file after merge. If not, don't worry.

Thanks again for your work on this. If you'd like any help addressing the comments, please let me know.

var hours = formattedTime[:hour];
var minutes = formattedTime[:min];
var amPmText = formattedTime[:amPm];
var colon = ":"; // SG Addition
Copy link
Owner

Choose a reason for hiding this comment

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

I'm afraid I shouldn't allow the colon and font change to the always-on display: the bold hours/light minutes display is a defining characteristic of Crystal's display, so I'd prefer to keep this as is. The time display should be consistent between regular, and always-on.

For reference, note that this change also affects the newly-supported venusq and venusqm watches, which are currently broken:

image

The line endings have changed throughout this file.

Would you mind reversing changes to this file, and also to resources-round-390x390/fonts/fonts.xml?

We could consider making this change settings-based in future, although I'm not sure how to resolve the inconsistency with the regular time display, as there's no way a colon would fit for some devices.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I've just noticed that the reason for making the always-on changes was to reduce battery drain. Is the saving due to using fewer fonts?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the bold characters for the hours has many more 'on' pixels than the ':' between the hours and minutes. Nevertheless, my watch went from 1% per hour tp 4-5% per hour with always on so I turned it off. I didn't want my watch to turn to an Apple watch requiring a daily charge lol.

Copy link
Author

Choose a reason for hiding this comment

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

I've checked all the 'AlwaysOnHoursFont' and made sure the ':' is present.

<string id="ThemeBlueDark">Bleu (sur fond noir)</string>
<string id="ThemePinkDark">Rose (sur fond noir)</string>
<string id="ThemeRedDark">Rouge (sur fond noir)</string>
<string id="ThemeGreenDark">Vert(sur fond noir)</string>
Copy link
Owner

Choose a reason for hiding this comment

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

Missing space:

Suggested change
<string id="ThemeGreenDark">Vert(sur fond noir)</string>
<string id="ThemeGreenDark">Vert (sur fond noir)</string>

<string id="Mar">Mar</string>
<string id="Apr">Avr</string>
<string id="May">Mai</string>
<string id="Jun">Juin</string>
<string id="Jul">Juil</string>
<string id="Aug">Aout</string>
<string id="Aug">Août</string>
Copy link
Owner

Choose a reason for hiding this comment

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

Many thanks for all these translations.

The accented û is not included in one or more date fonts, so appears incorrectly. Could you fix this?

image

Copy link
Author

Choose a reason for hiding this comment

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

Do you know which font is used or do simply cut and paste a standard 'u' and add a caret over it in PaintShopPro to make a û?

Copy link
Author

Choose a reason for hiding this comment

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

I think I've catched them all. Also fixed the ':' for low power mode Venu Sq.

@@ -0,0 +1,104 @@
<strings>
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this file, as I think it's not longer needed.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no problem

@@ -0,0 +1,730 @@
using Toybox.WatchUi as Ui;
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this file, as I think it's not longer needed.

@@ -88,6 +88,7 @@
<listEntry value="10">@Strings.SunriseSunset</listEntry>
<listEntry value="11">@Strings.Weather</listEntry>
<listEntry value="13">@Strings.Humidity</listEntry>
<listEntry value="14">@Strings.PulseOx</listEntry>
Copy link
Owner

Choose a reason for hiding this comment

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

Crystal should only show settings that are actually supported by a given device (see how e.g. Temperature and Floors are handled). currentOxygenSaturation is only supported by quite a limited set of devices - is there an easy way we can hide the setting from other devices?

Copy link
Author

Choose a reason for hiding this comment

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

I'll check

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean like for example the resource file resources-approachs62 has "listEntry value="1">@Strings.FloorsClimbed</listEntry" in comments? If so, would that mean a specific resource file for each device that supports Pulse Ox?

Copy link
Author

Choose a reason for hiding this comment

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

Because it's supported by quite a few devices now:

Captain Marvel
D2™ Air
Darth Vader™
Descent™ Mk2 / Descent™ Mk2i
Descent™ Mk2 S
Enduro™
First Avenger
Forerunner® 245
Forerunner® 245 Music
Forerunner® 745
Forerunner® 945
Forerunner® 945 LTE
fēnix® 5X Plus
fēnix® 6 / 6 Solar / 6 Dual Power
fēnix® 6 Pro / 6 Sapphire / 6 Pro Solar / 6 Pro Dual Power / quatix® 6
fēnix® 6S / 6S Solar / 6S Dual Power
fēnix® 6S Pro / 6S Sapphire / 6S Pro Solar / 6S Pro Dual Power
fēnix® 6X Pro / 6X Sapphire / 6X Pro Solar / tactix® Delta Sapphire / Delta Solar / Delta Solar - Ballistics Edition / quatix® 6X / 6X Solar / 6X Dual Power
MARQ™ Adventurer
MARQ™ Athlete
MARQ™ Aviator
MARQ® Captain / MARQ® Captain: American Magic Edition
MARQ™ Commander
MARQ™ Driver
MARQ™ Expedition
MARQ™ Golfer
Rey™
Venu™
Venu™ 2
Venu™ 2S

@@ -76,6 +76,7 @@
<string id="Weather">Weather</string>
<string id="Humidity">Humidity</string>
<string id="Pressure">Pressure</string>
<string id="PulseOx">Pulse Ox</string>
Copy link
Owner

Choose a reason for hiding this comment

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

Please also add this string to the remaining 19 translation files, so that it can be easily translated in future.

Copy link
Author

Choose a reason for hiding this comment

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

Done

activityInfo = Activity.getActivityInfo();
sample = activityInfo != null and activityInfo has :currentOxygenSaturation ? activityInfo.currentOxygenSaturation : null;
if (sample != null) {
value = sample.format(INTEGER_FORMAT);
Copy link
Owner

Choose a reason for hiding this comment

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

Do you think the PulseOx value should be shown with a trailing "%", like Humidity and Battery?

Copy link
Author

Choose a reason for hiding this comment

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

Would make sense, yes since it's a percentage

Copy link
Author

Choose a reason for hiding this comment

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

Done

@SylvainGa
Copy link
Author

Do you have a vector version of the O2 icon? If so, I'd like to update the crystal-icons.afdesign Affinity Designer file after merge. If not, don't worry.

Thanks again for your work on this. If you'd like any help addressing the comments, please let me know.

No, I'm sorry. I don't have a vectored graphic for the O2. I simply used PaintShop Pro and used the font Segoe UI to write O and 2 using two different font size.

@warmsound warmsound modified the milestones: 2.4.5, 2.4.6 Jan 12, 2021
@warmsound
Copy link
Owner

@SylvainGa, would you like any help with this? I'd be happy to merge as is, and then I could make further changes before release, along the above lines. Your changes are valuable, so it'd be great to get them in 👍

@SylvainGa
Copy link
Author

SylvainGa commented Jan 25, 2021 via email

@warmsound
Copy link
Owner

Sorry to hear about your accident. Wishing you a speedy recovery, and thank you again for your work on this - there's no urgent hurry.

@warmsound warmsound removed this from the 2.4.6 milestone Apr 11, 2021
@lrosenman
Copy link

I would love to see this make it into a release at some point.

@SylvainGa
Copy link
Author

I've started working on it again. Refreshed my laptop so had to refork and redo my changes (kept my source). See my question in your post about Pulse Ox

#184 (comment)

@SylvainGa
Copy link
Author

SylvainGa commented Mar 23, 2022 via email

@lrosenman
Copy link

I'm not the code owner.

@warmsound is.

@SylvainGa
Copy link
Author

SylvainGa commented Mar 23, 2022 via email

@SylvainGa
Copy link
Author

SylvainGa commented Mar 23, 2022 via email

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.

3 participants