Add cutomizable timer duration #319

pakaoraki · 2024-03-14T17:18:03Z

Add new "Timer" menu in preferences with customizable timer duration.

This will close #296

pakaoraki · 2024-03-14T17:22:06Z

It look like there is some issues with my code, I can't fix PR right now, I will have more time tomorrow evening. Cheers

stuarthayhurst · 2024-03-14T18:04:38Z

I haven't looked at the code yet, but would it be better to store timer lengths using 1 key as an array, instead of 3 separate keys?

Also, if I remember correctly, red is supposed to be for destructive actions according to the GNOME Human Interface Guidelines I think it was? I don't know if that applies to extensions, but it might be worth considering to be consistent with the rest of GNOME.

Still, this is a clear improvement over the current system, nice work :)

pakaoraki · 2024-03-15T12:52:57Z

I haven't looked at the code yet, but would it be better to store timer lengths using 1 key as an array, instead of 3 separate keys?

Yes I totally agree, I was thinking the same. I just used a simple 'int' type for each duration but it need to be unified with a single key.

Also, if I remember correctly, red is supposed to be for destructive actions according to the GNOME Human Interface Guidelines I think it was? I don't know if that applies to extensions, but it might be worth considering to be consistent with the rest of GNOME.

It make sense, I hesitated to keep the red color anyway so I can remove it.

There are some other small improvements to make, I'll try to fix it all this evening or tomorrow for final review.

pakaoraki · 2024-03-15T21:36:26Z

There is a weird eslint issue:

/home/runner/work/gnome-shell-extension-caffeine/gnome-shell-extension-caffeine/caffeine@patapon.info/preferences/timerPage.js
283:13 error Expected property shorthand object-shorthand

The code is the same as line 66, but it cause an error only in line 283. Should I ignore this using /* eslint-disable object-shorthand */ and /* eslint-enable object-shorthand */ or did I miss something ?

stuarthayhurst · 2024-03-15T21:53:29Z

There is a weird eslint issue:

/home/runner/work/gnome-shell-extension-caffeine/gnome-shell-extension-caffeine/caffeine@patapon.info/preferences/timerPage.js
283:13 error Expected property shorthand object-shorthand

The code is the same as line 66, but it cause an error only in line 283. Should I ignore this using /* eslint-disable object-shorthand */ and /* eslint-enable object-shorthand */ or did I miss something ?

I think it's because the variables have different names, durationIndex and value. I think changing the name of value, or replacing value: value with value should do it?

pakaoraki · 2024-03-15T21:59:55Z

Thanks

caffeine@patapon.info/extension.js

gabrbrand · 2024-03-26T12:57:04Z

caffeine@patapon.info/preferences/timerPage.js

+        let enableCustomTimerSwitch = new Gtk.Switch({
+            valign: Gtk.Align.CENTER,
+            active: this._settings.get_boolean(this._settingsKey.USE_CUSTOM_DURATION)
+        });
+        this.enableCustomTimerRow = new Adw.ActionRow({
+            title: _('Enable custom values'),
+            subtitle: _('Select custom value for each duration'),
+            activatable_widget: enableCustomTimerSwitch
+        });
+        this.enableCustomTimerRow.add_suffix(enableCustomTimerSwitch);


Suggested change

let enableCustomTimerSwitch = new Gtk.Switch({

valign: Gtk.Align.CENTER,

active: this._settings.get_boolean(this._settingsKey.USE_CUSTOM_DURATION)

});

this.enableCustomTimerRow = new Adw.ActionRow({

title: _('Enable custom values'),

subtitle: _('Select custom value for each duration'),

activatable_widget: enableCustomTimerSwitch

});

this.enableCustomTimerRow.add_suffix(enableCustomTimerSwitch);

this.enableCustomTimerRow = new Adw.SwitchRow({

title: _('Enable custom values'),

subtitle: _('Select custom value for each duration'),

active: this._settings.get_boolean(this._settingsKey.USE_CUSTOM_DURATION)

});

SwitchRow was introduced with libadwaita 1.4 (Gnome 45). It would make sense to use it here.

Thanks, I changed it.

gabrbrand · 2024-03-26T12:58:42Z

caffeine@patapon.info/preferences/timerPage.js

+
+        // Bind signals
+        // --------------
+        enableCustomTimerSwitch.connect('notify::active', (widget) => {


Suggested change

enableCustomTimerSwitch.connect('notify::active', (widget) => {

this.enableCustomTimerRow.connect('notify::active', (widget) => {

See above

gabrbrand · 2024-03-27T13:33:42Z

I manually installed this branch with make all on Fedora Rawhide (Gnome 46).
The stopwatch-symbolic currently isn't visible in dark style:

This can be fixed by moving caffeine@patapon.info/icons/stopwatch-symbolic.svg to caffeine@patapon.info/icons/hicolor/scalable/actions and changing

Gio.icon_new_for_string(`${this._path}/icons/${TimerMenuIcon}.svg`);

to

Gio.icon_new_for_string(`${this._path}/icons/hicolor/scalable/actions/${TimerMenuIcon}.svg`);

in caffeine@patapon.info/extension.js (line 147)

pakaoraki · 2024-03-27T17:39:14Z

Thanks for reporting, icon fixed. Tested in Ubuntu 24.04 / Fedora 40.

caffeine@patapon.info/extension.js

stuarthayhurst · 2024-04-17T14:27:00Z

The pipeline failure seems to be eslint 9 complaining about arguments, we wouldn't need to try and fix it if we just drop the jsdoc stuff, since I don't think we don't really use it anyway

If we drop it we just need to update the makefile, workflow definition and linter configs

pakaoraki · 2024-04-18T10:29:38Z

I tried to remove the jsdoc plugin but it's seams that we need to rewrite the entire Eslint config for version 9.0 (migrate-to-9.0.0) as they changed the config file format.

Should we make a separate PR for this ?

stuarthayhurst · 2024-04-18T12:55:56Z

Should we make a separate PR for this ?

Yeah, probably a good idea

matdave · 2024-04-29T15:53:14Z

This feature is awesome. I've been wanting long timers since I work on dual computers and need both on during my work day. This is saving me from having to remember to turn caffeine off at the end of my work day.

Display format hh:mm:ss

pakaoraki · 2024-05-26T17:49:29Z

@eonpatapon @stuarthayhurst
I think it's ready to merge, let me know if it's ok for you.

stuarthayhurst · 2024-05-26T18:43:45Z

Looks ok to me, do we want to merge this with a squash merge since there are a lot of small commits and fixes for previous commits?

Remove old code

Add missing leading zero and fix subtitle display hours Fix variable name Change countdown-timer to seconds

Clean durations title Clean code

Fix translations

Fix spaces and syntax issues Remove unused vars

Fix spacing Remove trailling space Fix object-shorthand Fix spacing

Fix object-shorthand with value Rename constant with uppercase Symplify code Replace Gtk.Switch/ActionRow with Adw.SwitchRow Improve code: get durations values Fix stopwatch icon Update translations

This reverts commit 89ec49c.

Update locales Update catalan translation

stuarthayhurst · 2024-06-11T14:29:25Z

caffeine@patapon.info/preferences/timerPage.js

+        this.sliderTimer.add_mark(0, Gtk.PositionType.BOTTOM, null);
+        this.sliderTimer.add_mark(1, Gtk.PositionType.BOTTOM, null);
+        this.sliderTimer.add_mark(2, Gtk.PositionType.BOTTOM, null);
+        this.sliderTimer.add_mark(3, Gtk.PositionType.BOTTOM, null);
+        this.sliderTimer.add_mark(4, Gtk.PositionType.BOTTOM, null);


Should this be a loop instead?

stuarthayhurst reviewed Mar 15, 2024

View reviewed changes

caffeine@patapon.info/extension.js Outdated Show resolved Hide resolved

stuarthayhurst reviewed Mar 16, 2024

View reviewed changes

caffeine@patapon.info/extension.js Outdated Show resolved Hide resolved

gabrbrand suggested changes Mar 26, 2024

View reviewed changes

pakaoraki force-pushed the change_timers branch from 31cf5cd to 79915b7 Compare March 27, 2024 12:09

stuarthayhurst reviewed Apr 14, 2024

View reviewed changes

caffeine@patapon.info/extension.js Outdated Show resolved Hide resolved

stuarthayhurst reviewed Apr 14, 2024

View reviewed changes

caffeine@patapon.info/extension.js Outdated Show resolved Hide resolved

pakaoraki force-pushed the change_timers branch 2 times, most recently from 7ef6741 to dd1222c Compare April 30, 2024 22:08

pakaoraki added 4 commits May 26, 2024 18:58

Add new preference page for timer

344c3dd

Add new keys

e5d59f1

Remove timer settings from general page

92fbb3e

Use new duration keys

45ec40c

Display format hh:mm:ss

pakaoraki force-pushed the change_timers branch from dd1222c to d806228 Compare May 26, 2024 17:35

pakaoraki added 3 commits May 27, 2024 12:46

Clean and remove old code

ca61f9b

Remove old code

Fix timer

f725468

Add missing leading zero and fix subtitle display hours Fix variable name Change countdown-timer to seconds

Replace Gtk.text with Gtk.entry

6d9dbc9

pakaoraki added 16 commits May 27, 2024 12:46

Set sensitive property on the ActionRow

9830e4a

Change reset icon

18c0ce7

Update translation

c6c9171

Clean code

cc901fa

Clean durations title Clean code

Fix catalan translation

6c6c424

Fix translations

Clean code and syntax

8a91c83

Fix spaces and syntax issues Remove unused vars

Rename declarative value

0785c4a

Fix syntax

19c19d6

Remove css style error on reset button

f5ded21

Use only one key to store duration

3c1022a

Add missing translations and update

b54c422

Fix syntax

2cbbf24

Fix spacing Remove trailling space Fix object-shorthand Fix spacing

Improve and fix code

e0728a8

Fix object-shorthand with value Rename constant with uppercase Symplify code Replace Gtk.Switch/ActionRow with Adw.SwitchRow Improve code: get durations values Fix stopwatch icon Update translations

Linter: remove jsdoc plugin

2b885e2

Revert "Linter: remove jsdoc plugin"

3fc6f86

This reverts commit 89ec49c.

Update translation

f749cdd

Update locales Update catalan translation

pakaoraki force-pushed the change_timers branch from d806228 to f749cdd Compare May 27, 2024 10:47

stuarthayhurst reviewed Jun 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cutomizable timer duration #319

Add cutomizable timer duration #319

pakaoraki commented Mar 14, 2024

pakaoraki commented Mar 14, 2024

stuarthayhurst commented Mar 14, 2024 •

edited

pakaoraki commented Mar 15, 2024

pakaoraki commented Mar 15, 2024

stuarthayhurst commented Mar 15, 2024

pakaoraki commented Mar 15, 2024

gabrbrand Mar 26, 2024

pakaoraki Mar 27, 2024

gabrbrand Mar 26, 2024

gabrbrand commented Mar 27, 2024 •

edited

pakaoraki commented Mar 27, 2024 •

edited

stuarthayhurst commented Apr 17, 2024

pakaoraki commented Apr 18, 2024

stuarthayhurst commented Apr 18, 2024

matdave commented Apr 29, 2024

pakaoraki commented May 26, 2024

stuarthayhurst commented May 26, 2024

stuarthayhurst Jun 11, 2024

	enableCustomTimerSwitch.connect('notify::active', (widget) => {
	this.enableCustomTimerRow.connect('notify::active', (widget) => {

Add cutomizable timer duration #319

Are you sure you want to change the base?

Add cutomizable timer duration #319

Conversation

pakaoraki commented Mar 14, 2024

pakaoraki commented Mar 14, 2024

stuarthayhurst commented Mar 14, 2024 • edited

pakaoraki commented Mar 15, 2024

pakaoraki commented Mar 15, 2024

stuarthayhurst commented Mar 15, 2024

pakaoraki commented Mar 15, 2024

gabrbrand Mar 26, 2024

Choose a reason for hiding this comment

pakaoraki Mar 27, 2024

Choose a reason for hiding this comment

gabrbrand Mar 26, 2024

Choose a reason for hiding this comment

gabrbrand commented Mar 27, 2024 • edited

pakaoraki commented Mar 27, 2024 • edited

stuarthayhurst commented Apr 17, 2024

pakaoraki commented Apr 18, 2024

stuarthayhurst commented Apr 18, 2024

matdave commented Apr 29, 2024

pakaoraki commented May 26, 2024

stuarthayhurst commented May 26, 2024

stuarthayhurst Jun 11, 2024

Choose a reason for hiding this comment

stuarthayhurst commented Mar 14, 2024 •

edited

gabrbrand commented Mar 27, 2024 •

edited

pakaoraki commented Mar 27, 2024 •

edited