vorta icon indicating copy to clipboard operation
vorta copied to clipboard

feat: settings to allow new networks and show notif when a network is disallowed

Open diivi opened this issue 2 years ago • 10 comments
trafficstars

Adds 2 columns to the profile model:

  1. shows the "This wifi is not allowed" notification only when it is checked. Shows the notif by default.
  2. adds new networks to allowed networks only when checked. Does not add by default.

Related Issue

Fixes #1655

Motivation and Context

#1654

How Has This Been Tested?

Manually tested.

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] I have read the CONTRIBUTING guide.
  • [x] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [ ] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

diivi avatar Mar 17 '23 11:03 diivi

I feel like "Show notification when a network is disallowed" duplicates the option that's already there under Misc - "Display notification when background tasks fails". If you look at notifications and backups in general:

  1. It may succeed: we have a notification option for that
  2. It may fail for some random reason: we have a notification for that too
  3. It may fail due to disallowed network: this PR

The question is if 2 and 3 are different enough to have their own option. Or if 3 is just some other failure. In any case, the option is better suited under Misc as it's so similar to the others.

m3nu avatar Apr 05 '23 11:04 m3nu

@bcelary what do you think (about the difference between 2 and 3)? Since you requested this in the first place.

Otherwise, yeah, maybe this should be under Misc.

diivi avatar Apr 05 '23 11:04 diivi

Seems like misc is the right place to keep it consistent. It looks like this is really a different category of a message that is not really an error but like an info. Similarly there could be an error that there is no available network at all which seems pointless because user is likely aware of this (not sure if there's such a message but just as an example).

Caveat here that I'm not an UX expert.

bcelary avatar Apr 06 '23 09:04 bcelary

I think 2 and 3 can be merged to one notification.

real-yfprojects avatar Apr 06 '23 09:04 real-yfprojects

I think 2 and 3 can be merged to one notification.

So I should drop the second part of this PR entirely, and @bcelary can use the Misc - Display notification when background tasks fails setting to disable wifi notifications?

It looks like this is really a different category of a message that is not really an error but like an info

I agree; but a different setting for this use case probably only makes sense if there's an overlap, i.e. you want to hide an error, not the wifi notification (or vice versa), but end up hiding both because of the current setting.

diivi avatar Apr 06 '23 12:04 diivi

I realize that too many options is not ideal so I'm definitely good with whatever you decide.

Maybe some criticality should be added to notifications so that user would be able to decide the level of severity but that would require a new feature. I think the most sensible is to simply drop that part indeed.

bcelary avatar Apr 06 '23 13:04 bcelary

What should I do about this - https://github.com/borgbase/vorta/pull/1658#discussion_r1158378665?

diivi avatar Apr 07 '23 03:04 diivi

What should I do about this - #1658 (comment)?

Didn't we discuss that we treat blocked wifi like any other error?

real-yfprojects avatar Apr 08 '23 08:04 real-yfprojects

Can you upgrade to PyQt6 (#1685)?

real-yfprojects avatar Apr 29 '23 10:04 real-yfprojects

This is almost ready.

m3nu avatar Jun 23 '23 11:06 m3nu