Add color picker for PineTimeStyle watchface #458
Conversation
This reverts commit 378fa6b.
|
It looks like the default settings are not being applied and on first boot the colors are white/white/white, but I think I'm settings the default values in Settings.h at lines 133-135 ? If anyone has any ideas what I'm doing wrong.... |
|
I agree with @Riksu9000 : I think it would be more "scalable" if watchfaces and apps had their own settings "embedded" in the app instead of adding an entry to the global settings. I'll have a look at this PR anyway. I think we can still merge it with color picker in the setting page, and move it in the app on a future PR. |
|
I like the idea but I'm worried about accidental triggers, maybe an alternative would be to have a global watchface settings button which lets you change settings for each watchface below a single settings item? |
Indeed, that's another possibility. I guess we could build prototypes of those solutions to see which one works best. But it's not easy to figure out what would be the best as this color picker is the first "app specific" setting in InfiniTime.
I'll probably start the review later today, but I can wait a bit before merging if you plan on adding improvements soon ;) |
|
Random colors option seems a bit unnecessary. Also I haven't looked into this PR much, but does it conflict with the idea of having a selectable global accent color? I'm hoping some day there would be a single setting that can adjust colors in all screens, maybe including the watch face? |
That's probably a separate issue and should be discussed there. |
Sure, I just opened a new issue for it. Still it might conflict with this and require changing this color picker afterwards if it isn't taken into consideration, which is why I mentioned it. |
Yes randomisation is unnecessary but it's kinda nice, and easy enough to implement. You should try the PR, while I like the default colors I also enjoy just picking some random colors. Eventually we could end up with a list of popular combinations and have the randomise button scroll through them instead of being totally random, but for now it's a nice way to change all the colors at once and hopefully find a combo you like. edit: I think a system wide accent color is a nice idea but should exist separately to the watchface color selection. I don't necessarily want the 2 to be linked |
|
You were right. I tried it and it really is a fun feature :D. Maybe one of the color options should be to follow the accent color, and this would be the default. It might be weird if each time someone wanted to change the accent color it would have to be changed in multiple places. |
|
The side bar can be set to black, but then the text and icons aren't visible. |
Yes it can, but it won't be set to black by the randomiser, only manually. Having the default follow the accent color is a nice idea, just needs a system wide color picker implemented... For now is everyone ok with it as it stands, with a plan of implementing the color picker in some fashion for all watchfaces and potentially the system accent color too? |
| btnNextTime = lv_btn_create(lv_scr_act(), nullptr); | ||
| btnNextTime->user_data = this; | ||
| lv_obj_set_size(btnNextTime, 60, 60); | ||
| lv_obj_align(btnNextTime, lv_scr_act(), LV_ALIGN_IN_RIGHT_MID, -15, -80); |
There was a problem hiding this comment.
Maybe some of these buttons should be aligned to the corners with LV_ALIGN_IN_TOP_RIGHT for example
There was a problem hiding this comment.
They could, but I'm happy with the current approach
| lv_obj_set_style_local_radius(sidebar, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, 0); | ||
| lv_obj_set_size(sidebar, 40, 240); | ||
| lv_obj_align(sidebar, lv_scr_act(), LV_ALIGN_IN_TOP_RIGHT, 0, 0); | ||
|
|
||
| // Display icons | ||
|
|
||
| batteryIcon = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_text_color(batteryIcon, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_label_set_text(batteryIcon, Symbols::batteryFull); | ||
| lv_obj_align(batteryIcon, sidebar, LV_ALIGN_IN_TOP_MID, 0, 2); | ||
|
|
||
| bleIcon = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_text_color(bleIcon, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_label_set_text(bleIcon, Symbols::bluetooth); | ||
| lv_obj_align(bleIcon, sidebar, LV_ALIGN_IN_TOP_MID, 0, 25); | ||
|
|
||
| // Calendar icon | ||
|
|
||
| calendarOuter = lv_obj_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_bg_color(calendarOuter, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_obj_set_style_local_radius(calendarOuter, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, 0); | ||
| lv_obj_set_size(calendarOuter, 34, 34); | ||
| lv_obj_align(calendarOuter, sidebar, LV_ALIGN_CENTER, 0, 0); | ||
|
|
||
| calendarInner = lv_obj_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_bg_color(calendarInner, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0xffffff)); | ||
| lv_obj_set_style_local_radius(calendarInner, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, 0); | ||
| lv_obj_set_size(calendarInner, 27, 27); | ||
| lv_obj_align(calendarInner, calendarOuter, LV_ALIGN_CENTER, 0, 0); | ||
|
|
||
| calendarBar1 = lv_obj_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_bg_color(calendarBar1, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_obj_set_style_local_radius(calendarBar1, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, 0); | ||
| lv_obj_set_size(calendarBar1, 3, 12); | ||
| lv_obj_align(calendarBar1, calendarOuter, LV_ALIGN_IN_TOP_MID, -6, -3); | ||
|
|
||
| calendarBar2 = lv_obj_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_bg_color(calendarBar2, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_obj_set_style_local_radius(calendarBar2, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, 0); | ||
| lv_obj_set_size(calendarBar2, 3, 12); | ||
| lv_obj_align(calendarBar2, calendarOuter, LV_ALIGN_IN_TOP_MID, 6, -3); | ||
|
|
||
| calendarCrossBar1 = lv_obj_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_bg_color(calendarCrossBar1, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_obj_set_style_local_radius(calendarCrossBar1, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, 0); | ||
| lv_obj_set_size(calendarCrossBar1, 8, 3); | ||
| lv_obj_align(calendarCrossBar1, calendarBar1, LV_ALIGN_IN_BOTTOM_MID, 0, 0); | ||
|
|
||
| calendarCrossBar2 = lv_obj_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_bg_color(calendarCrossBar2, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_obj_set_style_local_radius(calendarCrossBar2, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, 0); | ||
| lv_obj_set_size(calendarCrossBar2, 8, 3); | ||
| lv_obj_align(calendarCrossBar2, calendarBar2, LV_ALIGN_IN_BOTTOM_MID, 0, 0); | ||
|
|
||
| // Display date | ||
|
|
||
| dateDayOfWeek = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_text_color(dateDayOfWeek, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_label_set_text(dateDayOfWeek, "THU"); | ||
| lv_obj_align(dateDayOfWeek, sidebar, LV_ALIGN_CENTER, 0, -34); | ||
|
|
||
| dateDay = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_text_color(dateDay, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_label_set_text(dateDay, "25"); | ||
| lv_obj_align(dateDay, sidebar, LV_ALIGN_CENTER, 0, 3); | ||
|
|
||
| dateMonth = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_text_color(dateMonth, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x000000)); | ||
| lv_label_set_text(dateMonth, "MAR"); | ||
| lv_obj_align(dateMonth, sidebar, LV_ALIGN_CENTER, 0, 32); | ||
|
|
||
| // Step count gauge | ||
| needle_colors[0] = LV_COLOR_WHITE; | ||
| stepGauge = lv_gauge_create(lv_scr_act(), nullptr); | ||
| lv_gauge_set_needle_count(stepGauge, 1, needle_colors); | ||
| lv_obj_set_size(stepGauge, 40, 40); | ||
| lv_obj_align(stepGauge, sidebar, LV_ALIGN_IN_BOTTOM_MID, 0, 0); | ||
| lv_gauge_set_scale(stepGauge, 360, 11, 0); | ||
| lv_gauge_set_angle_offset(stepGauge, 180); | ||
| lv_gauge_set_critical_value(stepGauge, (100)); | ||
| lv_gauge_set_range(stepGauge, 0, (100)); | ||
| lv_gauge_set_value(stepGauge, 0, 0); | ||
|
|
||
| lv_obj_set_style_local_pad_right(stepGauge, LV_GAUGE_PART_MAIN, LV_STATE_DEFAULT, 3); | ||
| lv_obj_set_style_local_pad_left(stepGauge, LV_GAUGE_PART_MAIN, LV_STATE_DEFAULT, 3); | ||
| lv_obj_set_style_local_pad_bottom(stepGauge, LV_GAUGE_PART_MAIN, LV_STATE_DEFAULT, 3); | ||
| lv_obj_set_style_local_line_opa(stepGauge, LV_GAUGE_PART_MAIN, LV_STATE_DEFAULT, LV_OPA_COVER); | ||
| lv_obj_set_style_local_scale_width(stepGauge, LV_GAUGE_PART_MAIN, LV_STATE_DEFAULT, 4); | ||
| lv_obj_set_style_local_line_width(stepGauge, LV_GAUGE_PART_MAIN, LV_STATE_DEFAULT, 4); | ||
| lv_obj_set_style_local_line_color(stepGauge, LV_GAUGE_PART_MAIN, LV_STATE_DEFAULT, LV_COLOR_BLACK); | ||
| lv_obj_set_style_local_line_opa(stepGauge, LV_GAUGE_PART_NEEDLE, LV_STATE_DEFAULT, LV_OPA_COVER); | ||
| lv_obj_set_style_local_line_width(stepGauge, LV_GAUGE_PART_NEEDLE, LV_STATE_DEFAULT, 3); | ||
| lv_obj_set_style_local_pad_inner(stepGauge, LV_GAUGE_PART_NEEDLE, LV_STATE_DEFAULT, 4); | ||
|
|
||
| backgroundLabel = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_click(backgroundLabel, true); | ||
| lv_label_set_long_mode(backgroundLabel, LV_LABEL_LONG_CROP); | ||
| lv_obj_set_size(backgroundLabel, 240, 240); | ||
| lv_obj_set_pos(backgroundLabel, 0, 0); | ||
| lv_label_set_text(backgroundLabel, ""); |
There was a problem hiding this comment.
Is it possible to somehow use some of the code from the real watchface?
There was a problem hiding this comment.
It would be nice but I don't see how I could. If the settings becomes integrated with the watchface it should be possible.
| lv_obj_set_style_local_text_color(timeDD1, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, pts_colors[11]); | ||
| lv_obj_set_style_local_text_color(timeDD2, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, pts_colors[11]); | ||
| lv_obj_set_style_local_text_color(timeAMPM, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, pts_colors[11]); | ||
| settingsController.SetPTSColorBar(11); | ||
| lv_obj_set_style_local_bg_color(sidebar, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, pts_colors[11]); | ||
| settingsController.SetPTSColorBG(3); | ||
| lv_obj_set_style_local_bg_color(timebar, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, pts_colors[3]); |
There was a problem hiding this comment.
There should probably be an enum for pts_colors[]. Or maybe some other solution
There was a problem hiding this comment.
I'm afraid I don't know what an enum is or what advantage it offers, but the main reason it's an array is because i only store the array index in settings rather than the color
There was a problem hiding this comment.
With an enum, this could be pts_colors[colorBlack] and SetPTSColorBG(colorBlack) for example, which is clearer than 3 or 11, but there may be some even better way to do it.
I would still consider it a bug. |
Ok, I added a check to avoid the sidebar being set to black |
|
hi, |
|
The sidebar and text can happily be the same color, as can the sidebar and background. the only cases I want to avoid are the main time text being the same as the background, IE invisible, or the sidebar being black which again makes things invisible. in the future it might be possible to set the sidebar black and icons white, but not yet, so until then I think the checks implemented give the most freedom possible to the user while avoiding making things unreadable. Anyway, I've been sat here tapping random and skipping around for ages, it works well. |
|
@kieranc could you give a updated dfu here or on discord? |
|
Here you go! |
it should be pretty simple to add a check in PineTimeStyle |
|
The calendar icon would also need inverting which is less trivial, and the step count gauge.... |
|
Thanks @kieranc for this PR, and everyone for the review! I pushed the these changes just after merging the branch to add some improvements. |
I expected this to take a lot longer, but it works and I'm pretty happy with it so I'd like some input/review.
I'd like to make the buttons bigger/transparent as they're a bit hard to hit but that's about it.
The colors are stored in settings as an integer which is the index value of an array of colors stored as a variable in the class.
edit: The color picker adds about 3k to the DFU size