Skip to content

Combine Date and Time Settings #1465

Merged
Riksu9000 merged 14 commits into
InfiniTimeOrg:developfrom
Elements6007:combine
Jan 14, 2023
Merged

Combine Date and Time Settings #1465
Riksu9000 merged 14 commits into
InfiniTimeOrg:developfrom
Elements6007:combine

Conversation

@Elements6007
Copy link
Copy Markdown
Contributor

@Elements6007 Elements6007 commented Nov 27, 2022

Previous PR #1386
changes since previous PR:

197371890-4d6f5958-62b3-413c-aa6c-1323f0c64b9b.mp4

(please excuse my clumsy hands)

Works with InfiniSim after a few small changes. ill open a PR over there later.
unnamed (1)unnamed unnamed (2)

Thanks :)

FINAL:
InfiniSim_2023-01-02_160904

renames SettingSetDate.cpp to SetingSetDateTime.h & SettingSetDate.h to SettingSetDateTime.h

deletes SettingSetTime
@Riksu9000
Copy link
Copy Markdown
Contributor

These screens are related so condensing the options seems like a good idea, however I'm not sure about the scrollbar being the only indicator here.

How about pressing the option first brings up SetDate, and when you press Set, it moves to the SetTime screen, which then exits back to the settings menu?

@Elements6007
Copy link
Copy Markdown
Contributor Author

These screens are related so condensing the options seems like a good idea, however I'm not sure about the scrollbar being the only indicator here.

How about pressing the option first brings up SetDate, and when you press Set, it moves to the SetTime screen, which then exits back to the settings menu?

Something like this?

PXL_20221202_003146180_2.1.mp4

I could not figure out a way to get it to exit to the settings screen once the time is set(I'm still a noob).

There is, however, one possible issue(more like just a nuisance). What if a user wanted to just set the date? After pressing Set it would go to SetTime, then the user would have to do two down gestures to exit, instead of only one.

In my opinion the scrollbar is sufficient, but im more than happy to change it if y'all think its not enough :)

@Riksu9000
Copy link
Copy Markdown
Contributor

Actually I think the issue is that swiping to move to another screen that is not a list seems a bit off to me. By making it proceed by pressing Set, you also wouldn't need the scrolling at all. I can implement this idea to try if you don't mind.

@Elements6007
Copy link
Copy Markdown
Contributor Author

Elements6007 commented Dec 3, 2022

Actually I think the issue is that swiping to move to another screen that is not a list seems a bit off to me. By making it proceed by pressing Set, you also wouldn't need the scrolling at all. I can implement this idea to try if you don't mind.

Ok, so i was able to get it to work. what I have is screens.OnTouchEvent(Pinetime::Applications::TouchEvents::SwipeUp); in void SetDate to bring it to the SetTime screen, and in void SetTime I have running = false; to return it to the menu. I do not know if its the proper way of implementing this, but if you think its adequate ill commit it. Im also just going to keep the swipe feature as i see it does no harm.

@Elements6007
Copy link
Copy Markdown
Contributor Author

@Riksu9000 If you could check out the last commit that would be great :)

@Riksu9000
Copy link
Copy Markdown
Contributor

It's close to what I had in mind. The disabling of the button should be removed and I personally wouldn't make these screens a screenlist. I think it adds to the clutter when you could just press the set button to proceed. Then this feature shouldn't require many lines changed either.

@Elements6007
Copy link
Copy Markdown
Contributor Author

Elements6007 commented Dec 8, 2022

true, if its not a screenlist then its unnecessary clutter. I made a kind of draft commit of another way of doing this here. This way there is minimal changes, no file renames, and very simple. However there may be a few issues with this approach. First, it might be confusing looking through to find two files for seemingly one application(at least it would be for me). Second, since there is no scrollbar there is a lack of indication that SetTime exists(besides the title "Date&Time").

I could combine SetDate and SetTime into one file like in this PR, but unfortunately i cant figure out how to. I could also add something like the page indicator widget to fix the second possible issue i mentioned, but i think it would turn out pretty janky.

@Riksu9000
Copy link
Copy Markdown
Contributor

Riksu9000 commented Dec 8, 2022

Now that I think about it, I think what felt weird about this was just the scrollbar. What if instead we had page indicator dots on the side? I think we should try and see how that would look. We might be able to keep this code.

@Elements6007
Copy link
Copy Markdown
Contributor Author

so keep it as a screenlist and add dots like this?
Screenshot 2022-12-08 131922new
Screenshot 2022-12-08 131945

@Riksu9000
Copy link
Copy Markdown
Contributor

Yeah something like that. Maybe using white and gray for the colors.

@Elements6007
Copy link
Copy Markdown
Contributor Author

Elements6007 commented Dec 16, 2022

Yeah something like that. Maybe using white and gray for the colors.
PXL_20221216_023943303_2

so heres it with the dot indicators. However, i put these indicators in the setdatetime file. I tried really hard to make it a widget but i cant seem to find how to do make it work(especially making multiple of the same lv objects), if you could work on it or show me what to do that would be great :)

Im really sorry im not proficient enough to work on the dot indicators further and I fully understand if you dont have the time to work on this.

@Riksu9000 Riksu9000 self-assigned this Dec 16, 2022
@Riksu9000
Copy link
Copy Markdown
Contributor

That's okay. I understand it may have been too much to ask. I can work on the widget at some point and I'll report back once it's ready.

@Elements6007
Copy link
Copy Markdown
Contributor Author

Elements6007 commented Dec 17, 2022

@Riksu9000 so i had sort of an epiphany and was able to figure it out after i took a fresh look at it, turns out it was pretty simple. It still has to be fine tuned to make it center it more. I dont know if it should be in a separate file(how it is in the commit) or as one file in DotLabel, it works either way. Anyway, I hope you see this before you start working on it!

Comment thread src/displayapp/widgets/DotIndicator.cpp Outdated
Comment thread src/displayapp/widgets/DotIndicator.cpp Outdated
Comment thread src/displayapp/widgets/DotIndicator.h Outdated
Comment thread src/displayapp/screens/settings/SettingSetDateTime.h Outdated
Comment thread src/displayapp/screens/settings/SettingSetDateTime.cpp Outdated
adds dot indicators and removes disabled set button. previous commit contained unwanted changes.
adds set scroll and removes SetDate from displayapp
@Elements6007
Copy link
Copy Markdown
Contributor Author

@Riksu9000 done :)

Comment thread src/displayapp/widgets/DotIndicator.cpp Outdated
Comment thread src/displayapp/screens/DotLabel.cpp Outdated
Comment thread src/displayapp/DisplayApp.cpp
Comment thread src/displayapp/screens/settings/SettingSetDateTime.cpp
move from setdate and settime to setdatetime also optimize DotIndicators

Co-Authored-By: Riku Isokoski <riksu9000@gmail.com>
@Elements6007
Copy link
Copy Markdown
Contributor Author

@Riksu9000 is it good now?

Copy link
Copy Markdown
Contributor

@Riksu9000 Riksu9000 left a comment

Choose a reason for hiding this comment

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

Looks good, just one comment. I'll approve this ahead of time. Thanks for the PR.

Comment thread src/displayapp/screens/settings/SettingSetDate.cpp Outdated
@Riksu9000 Riksu9000 requested a review from a team January 2, 2023 07:01
@Riksu9000 Riksu9000 added this to the 1.12.0 milestone Jan 2, 2023
@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Jan 4, 2023

I've just tested this PR. Combine those 2 settings in a single one is a good idea, and it looks good!

However, the app did not jump automatically to "set time" when I tap the button "Set" under the date setting. I also cannot see the page indicator in those settings and the new widget DotLabel does not seem to be used anywhere in the code. Is that expected?

@Riksu9000
Copy link
Copy Markdown
Contributor

@JF002 Did you use an older version of this PR? There's no issue for me.

@Elements6007
Copy link
Copy Markdown
Contributor Author

I've just tested this PR. Combine those 2 settings in a single one is a good idea, and it looks good!

However, the app did not jump automatically to "set time" when I tap the button "Set" under the date setting. I also cannot see the page indicator in those settings and the new widget DotLabel does not seem to be used anywhere in the code. Is that expected?

Works for me. Also DotLabel was removed in 992333c

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Jan 4, 2023

Oh... I kinda forgot to update my local branch... sorry for the confusion! Everything seems to work as expected!

@Riksu9000 Riksu9000 merged commit a7f8b59 into InfiniTimeOrg:develop Jan 14, 2023
@Elements6007
Copy link
Copy Markdown
Contributor Author

Elements6007 commented Jan 15, 2023

Thanks for all your help and patience on this @Riksu9000, i really appreciate it! I learned alot :)

NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request Jan 15, 2023
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