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

[[email protected]] Make Pomodoro completion more obvious. #6545

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

Conversation

cmcl
Copy link

@cmcl cmcl commented Oct 31, 2024

Extended short break dialogs to pop-up when a pomodoro has been completed and it is time for a short break. Combined this new functionality with the existing options for automatically continuing after a pomodoro and/or short break. In the latter case, the dialog is closed automatically. The dialog transitions appropriately when the timer expires.

@rcalixte
Copy link
Member

cc @gfreeau

@gfreeau
Copy link
Contributor

gfreeau commented Oct 31, 2024

@cmcl Thanks for working on the applet. I need you to make a few changes so it's easier to review:

  • A lot of the diff is whitespace changes, please get rid of this so easier to focus on the changed code
  • Remove the terminology changes to Vine, I'd like to just review this PR for the changes to the short break dialogues.

Extended short break dialogs to pop-up when a pomodoro has been
completed and it is time for a short break.  Combined this new
functionality with the existing options for automatically continuing
after a pomodoro and/or short break.  In the latter case, the dialog is
closed automatically.  The dialog transitions appropriately when the
timer expires.
@cmcl
Copy link
Author

cmcl commented Nov 1, 2024

Thanks @gfreeau, good points! I reverted the terminology and removed the whitespace changes.

I also fixed an oversight with resetting the short break dialog prompt and buttons.

@gfreeau
Copy link
Contributor

gfreeau commented Nov 10, 2024

@cmcl
I tested your changes and can see the benefit of the feature. I'd need you do more debugging before we could merge this though because with some clicking around I was able to get your changes to produce bugs with the count down timers and dialog windows, it seemed to be skipping more than 1 second at times and also showing 2 dialogs when only 1 was required. There has to be race condition bugs or similar in here.

Here is the testing I did:

In applet.js changes the convertMinutesToSeconds function to

function convertMinutesToSeconds(minutes) {
    return minutes;
}

This makes 25 minutes be 25 seconds instead for faster testing.

With the timers active, do things like

  • Click applet menu and use skip to next timer option
  • When short break dialog appears, click different options each time, some times skip the break, other time start the break. Do the same with long break dialog.

Basically whatever options you click, the timer behaviour should be able to handle it. I recommend doing at least 3 full pomodoro sets which is 3 long breaks to verify everything is working correctly.

These are the settings I am using:
image

@cmcl
Copy link
Author

cmcl commented Nov 20, 2024

Thanks @gfreeau for your diligence in testing my changes. I, too, noticed there was some undesirable interactions between the menu items and the buttons, possibly the same one you have noted here (I have it written down but don't remember off-hand). I'm writing just to say I'm aware of them and intend to update this pull request with fixes when I have a bit more time to work on it.

I am also curious if there is a more systematic way to test the interactions of the UI elements. It is quite difficult to keep all the possible permutations of button clicks, menu selects and ordering events in mind at once. It is also rather inefficient since one small change would require to re-check all those possibilities again for interference. Is there a community-wide accepted best practice on this issue?

@rcalixte rcalixte marked this pull request as draft November 20, 2024 01:06
@gfreeau
Copy link
Contributor

gfreeau commented Nov 20, 2024

@cmcl I have thought about this too in the past, I would like more comprehensive testing and leveraging industry standard tools from node/typescript community. It can be a challenge sometimes with cinnamon js.

In the past when I was looking into this, I came across https://github.com/linuxmint/cinnamon-spices-applets/tree/master/radio%40driglu4it/tests as one of the very few applets attempting a testing approach. Have a look and see what you think.

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