elfenware / badger

Remind yourself to not sit and stare at the screen for too long
GNU General Public License v3.0
53 stars 13 forks source link

Add per timer checkbox (#22) #32

Closed andreasomaini closed 4 years ago

andreasomaini commented 4 years ago

Working

Add a per timer checkbox in addition to the global switch (closes #22).

The notification logic works perfectly fine: a notification only appears if the global switch is active and if the corresponding checkbox is checked.

Problems

Because of how the binding between the scales and the global switch works, there are still two minor front-end problems:

Global switch off => checkboxes sensitive

switchoff

Since the checkboxes are bound to a new option property (eyes-active, fingers-active [...]) they cannot be bound to the global switch 'all' property.

Checkbox unchecked && global switch on => Scale sensitive

switchon

Same applies here, since the Scale sensitive property is bound to the 'all' property of the global switch, it cannot be controlled by the checkboxes.

dar5hak commented 4 years ago

@andreasomaini Amazing work! And good thinking replacing Never with 1 min since the former no longer makes sense.

As for the sensitivity issues, see if bind_with_mapping could be useful.

If that doesn't work, then we'll have to replace the binding with a manual event handler, which will be a lot of boilerplate code.

andreasomaini commented 4 years ago

With the latest commit ( d64fdd8 ) the first problem got fixed just by changing some binding flags.

I guess the other problem can be solved only by changing the sensitive state manually, since it is already bound to the all property.

dar5hak commented 4 years ago

Good finding about the flag. As for the second problem, I guess there really is no other way. We'll have to go with manual handling.

andreasomaini commented 4 years ago

Yes I just had to handle the scales' sensitive state manually. Now everything works and looks just fine, as it should be :)

dar5hak commented 4 years ago

@andreasomaini There's one more thing we'll need to handle. I didn't think of it before, but it struck me when I was testing your branch.

Previously, I had a few reminders set to "Never", meaning the GSettings key said 0 for those. With this feature coming in, we'll have to change those zeroes to unchecked boxes.

I'm thinking we could reset the intervals to the default values mentioned in Application.vala, and making the -active key false for them. However, if you have a better/easier idea, I'm listening.

The migration code for this could go into the already existing migration block in MainGrid.vala.

dar5hak commented 4 years ago

Also, I'm gonna try and find out why the Houston build is falling flat on its face.

andreasomaini commented 4 years ago

Previously, I had a few reminders set to "Never", meaning the GSettings key said 0 for those. With this feature coming in, we'll have to change those zeroes to unchecked boxes.

I actually hadn't thought about it, that's a problem for sure.

I'm thinking we could reset the intervals to the default values mentioned in Application.vala, and making the -active key false for them. However, if you have a better/easier idea, I'm listening.

I am not sure what you are referring to inside Application.vala. I guess we could just check the values in GSettings: if they are out of the [1, 60] range, set it to some default value and uncheck the checkbox, like this:

Before (line 125 MainGrid.vala)

uint interval = settings.get_uint (reminder.name);
scale.set_value (interval);

After

uint interval = settings.get_uint (reminder.name);
if ( interval <= 0 || interval >= 60 ) {
    // [**]
}
scale.set_value (interval);

Also, I'm gonna try and find out why the Houston build is falling flat on its face.

I had the same problem on this PR, couldn't find any solution actually, seems like a container image got removed somewhere (?)

Have you changed anything? The build is successful now.

dar5hak commented 4 years ago

if they are out of the [1, 60] range, set it to some default value

Yes, that's what I'm talking about. Although I'd just check for the lower bound and not bother with the upper bound. >60 has never been possible officially, and if some nerd did open up GSettings and make it >60, then I don't see a reason to mess with their configuration. Because nerds are awesome people.

As for the default values we want to set to, refer to Application.vala. Scroll way down and you'll see the initial intervals for each reminder. I think these would be good choices for resetting.

Have you changed anything? The build is successful now.

I changed nothing. Seems like Houston itself had a problem, as seen in your other PR. All good now.

dar5hak commented 4 years ago

Moreover, since the intervals are uint, you could just check for == 0 instead of <= 0.

andreasomaini commented 4 years ago

In the last commit, I just checked for that specific condition, reset to the default value specified in the schema, and updated the checkbox value.

dar5hak commented 4 years ago

@andreasomaini Thank you so much for implementing this useful feature!