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.

Sign up for GitHub

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

Jump to bottom

Feature/1457 add shorcuts for pause breaks #1468

Merged
merged 15 commits into from
Jul 25, 2024
Merged

Feature/1457 add shorcuts for pause breaks #1468

merged 15 commits into from
Jul 25, 2024

Conversation

evgenyneu
Copy link
Contributor

@evgenyneu evgenyneu commented Jul 23, 2024 •
edited
Loading

Issue: #1457

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • issue was opened to discuss proposed changes before starting implementation. It is important do discuss changes before implementing them (Why should we add it? How should it work? How should it look? Where will it be? ...).
  • during development, node version specified in package.json was used (ie using nvm).
  • package versions and package-lock.json were not changed (npm install --no-save).
  • app version number was not changed.
  • all new code has tests to ensure against regressions.
  • npm run lint reports no offenses.
  • npm run test is error-free.
  • README and CHANGELOG were updated accordingly.
  • [] after PR is approved, all commits in it are squashed

Description of the Change

  • Added ability to set keyboard shortcuts to pause the breaks for the period of time (30 min, 1 hr etc.)
  • Moved the code for keyboard chortcut registration from main.js into utils/breakShortcuts.js.

Verification Process

  • Open About tab in the Preferences screen.
  • Click Ctrl-D to see the path to preferences JSON
  • Set the keyboard shortcuts:
"pauseBreaksFor30MinutesShortcut": "CmdOrCtrl+Alt+0",
"pauseBreaksFor1HourShortcut": "CmdOrCtrl+Alt+1",
"pauseBreaksFor2HoursShortcut": "CmdOrCtrl+Alt+2",
"pauseBreaksFor5HoursShortcut": "CmdOrCtrl+Alt+5",
"pauseBreaksUntilMorningShortcut": "CmdOrCtrl+Alt+M",

  • Restart the app.

  • Use the keyboard shortcuts and see that the breaks are paused: the app icon changes to Pause and when you click the text shows the correct pause duration.

  • Since I moved the code for other break shortcuts into, I also tested these shortcuts:

"pauseBreaksToggleShortcut": "CmdOrCtrl+Alt+T",
"skipToNextScheduledBreakShortcut": "CmdOrCtrl+Alt+B",
"skipToNextMiniBreakShortcut": "CmdOrCtrl+Alt+S",
"skipToNextLongBreakShortcut": "CmdOrCtrl+Alt+L",
"resetBreaksShortcut": "CmdOrCtrl+Alt+R",
  • I tested it on Mac, Windows and Ubuntu.

evgenyneu added 4 commits July 23, 2024 16:09
@evgenyneu
Fix test
8c81987 
Update timezone in unit tests to make test pass.
Test failed when I ran them in
Melbourne because the hardcoded time stamps
were created in my local timezone (UTC+10)
while the test expectation
assume the times are in Amsterdam's time zone (UTC+2)
@evgenyneu
Add keyboard shortcuts for pause breaks
07ddfe2
@evgenyneu
Document
5947189
@evgenyneu
Changelog
ee9665f
hovancik
hovancik reviewed Jul 23, 2024
View reviewed changes
app/main.js Outdated
@@ -1315,26 +1317,31 @@ function getTrayMenuTemplate () {
submenu: [
{
label: i18next.t('utils.minutes', { count: 30 }),
accelerator: settings.get('pauseBreaksFor30MinutesShortcut'),
Copy link
Owner

Choose a reason for hiding this comment

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

I guess not that important but returns [18960:0723/192752.999383:WARNING:accelerator_util.cc(65)] doesn't contain a valid key when no value set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted. How do I reproduce this error? I tried to start the app (npm run start) with empty string "" values for the shortcuts but I don't see this error in the terminal.

Copy link
Owner

Choose a reason for hiding this comment

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

Try with npm run dev, it gives more debug info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, the warning only appeared in Ubuntu for me but not in Windows or Mac.

hovancik
hovancik reviewed Jul 23, 2024
View reviewed changes
app/utils/pauseBreaksShortcut.js Outdated
@@ -0,0 +1,44 @@
const { UntilMorning } = require('./untilMorning')
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please what do you think about registering pauseBreaksToggleShortcut? Haven't had time think myself, but it hits me we have one shortcut left in main.js regarding to pausing.

Or even put all shortcuts here? (like skipToNextScheduledBreakShortcut, skipToNextMiniBreakShortcut and such).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@hovancik
Copy link
Owner

I've made some comments, let me know what you think :)

I can test then on Windows as well.

@evgenyneu evgenyneu requested a review from hovancik July 25, 2024 03:58
@evgenyneu
Copy link
Contributor Author

I've made the improvements you suggested. Also, I managed to install the dev environment on Windows, and tested the shortcuts on all platforms. Let me know if you want to change/improve anything. :D

hovancik
hovancik approved these changes Jul 25, 2024
View reviewed changes
Copy link
Owner

@hovancik hovancik left a comment

Choose a reason for hiding this comment

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

Many thanks, looks good :)

@hovancik
Copy link
Owner

I can squash and merge, unless you want to add yourself to the readme :)

@evgenyneu
Copy link
Contributor Author

Thanks, squash is ok, I don't need readme mention :D

@hovancik hovancik merged commit e256b01 into hovancik:trunk Jul 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hovancik hovancik hovancik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@evgenyneu @hovancik