Make entire SettingsRow toggle the setting (closes #455) #466

tomjschwanke · 2020-06-15T12:40:26Z

(put this in a new PR to not merge from master into dev)

fixes #455

Checklist

Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
If this PR comes from a fork, please Allow edits from maintainers
Set a speaking title. Format: {task_name} (closes #{issue_number}). For example: Use logger (closes # 41)
Link your Pull Request to an issue (if applicable)
Create Work In Progress [WIP] pull requests only if you need clarification or an explicit review before you can continue your work item.
Make sure that your PR is not introducing unnecessary reformatting (e.g., introduced by on-save hooks in your IDE)
Make sure that your PR does not contain changes in strings.xml (see issue [HELP WANTED][README] Text / Translations #332)
Make sure that your PR does not contain compiled sources (already set by the default .gitignore) and / or binary files

Description

#455 mentioned the settings being only toggleable by hitting the switch. This PR makes the entire SettingsRow toggle the switch, making it behave like stock Android settings.

…t the SettingsSwitchRowSwitch

tomjschwanke · 2020-06-15T12:42:52Z

(copy from the other PR, but still relevant)
This would need to be remembered for all settings added in the future

Todo: maybe fix the animation, stock Android has a little animation, filling the box from where you tapped. In the current implementation, it lights up the entire box immediately

harambasicluka · 2020-06-17T10:14:23Z

Hi @tomjschwanke ,

thanks for your contribution! But it isn't as easy as it's in your PR, at least for the settings tracing switch. We already had your proposal implemented but removed it because we had to introduce more complexity to the switch and the row. And to be consistent we also removed it for the simpler notification switch rows.

So with the clickable row states like this can occur - the text says Stopped but the switch is On.

Reproduce:

Start with Active Exposure Logging
Disable Bluetooth
Click on the row
Tracing is Stopped and the switch is Off
Click on the switch (not the row)
Result: The text says Stopped but the switch is On

That's only one wrong state maybe there are others. If you can fix this I'm happy to include your PR, but you would have to provide a list of manual tests. After this I'll verify that everything is working like expected and also that accessibility isn't broken with this new click target.
I also included the logic for the switch, images and text states, can be found here.

The following table explains the different stati which can appear in the ui.
This follows this prioritization: Tracing, Bluetooth, Connection will only be relevant in one exact case, Bluetooth is relevant in two different cases, but independently from the Connection status. And in every other case Tracing will be shown.

Tracing	Bluetooth	Connection	Result
OFF	OFF	ON	TRACING*
OFF	ON	OFF	TRACING*
OFF	ON	ON	TRACING*
OFF	OFF	OFF	TRACING*
ON	ON	ON	TRACING
ON	OFF	ON	BLUETOOTH
ON	OFF	OFF	BLUETOOTH
ON	ON	OFF	CONNECTION

*circle has to be disabled via another formatter

If you aren't willing to do this please close this PR.

Cheers,
Luka

PS: To avoid misunderstandings: we would like to have this in, but only if it works as expected :)

tomjschwanke · 2020-06-17T14:00:29Z

Ahh I get it. Hitting the switch doesnt work now, as the switch doesn't have an OnClickListener anymore (thats on the Row now). This affects all switches.

We'd just need to set an OnClickListener on both and update the other accordingly, I will look into this.

I can't follow your steps, as I am unable to sign the app with the correct key (and therefore can't enable tracing), so once I come up with a potential fix, you'd need to test it.

…rking. Switches AND Rows now have an OnClickListener to change the setting.

tomjschwanke · 2020-06-17T16:19:37Z

I have changed it back to the way it was before, but added additional OnClickListeners for the Rows. Notification settings now toggle when clicking either on the Row or on the Switch. The tracing switch should behave the same. Before, I was able to change the Switch but it had no effect (as there was no OnClickListener bound to it) but now when I hit the switch, it gives me the Error message (because it's not signed).

The only issue I still have right now is, when the user slides the switch instead of clicking on it, the setting (+text) doesn't change, this issue exists in the production version too, however. I'll look into this.
This is being tracked in #607

tomjschwanke · 2020-06-18T17:30:48Z

Scanned with Accessibility scanner by Google

~~As you can see, it does not interfere with touch-targets, the switches remain the only ones (which should maybe actually be changed, the apps suggestion is to make the touch targets bigger).~~
I actually don't know if it shows all touch targets or only the ones that have suggestions.

I have already merged the switch sliding fix into this locally to test it, and everything is working.
Tapping on the row --> Option toggled
Tapping on the switch --> Option toggled
Sliding the switch --> Option toggled if position changed

tomjschwanke · 2020-06-18T18:21:31Z

When using talkback, tapping on the settings row says "on" / "off" depending on the switch position. It only reads the corresponding text on the second tap and every tap afterwards (until going back and going into the settings again). This also happens in the production version, so this PR doesn't change that.

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/settings/SettingsNotificationFragment.kt

harambasicluka · 2020-06-22T15:11:08Z

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/settings/SettingsTracingFragment.kt

@@ -101,6 +102,10 @@ class SettingsTracingFragment : Fragment(),
        switch.setOnClickListener {
            startStopTracing()
        }
+        // Additional click target to toggle switch
+        row.setOnClickListener {
+            startStopTracing()


same as above

implemented in commit 0dc17a1

the check has to be added here

harambasicluka · 2020-06-22T15:13:11Z

It seems to work fine with Talkback :)

…r) checking if row is enabled before executing action

tomjschwanke · 2020-06-22T17:07:47Z

~~I have no idea why it fails the check, it's up to date with dev and quickbuild on dev passes, while on here it complains about deprecated features being used~~
It was a missing space

harambasicluka · 2020-06-23T09:53:58Z

Thank's for the fast implementation, the builds due to a missing space.

Click on Details on Circle CI in this PR

Than you can open this Report: https://2305-268027139-gh.circle-artifacts.com/0/reports/ktlint/ktlintMainSourceSetCheck.txt

To reproduce this locally run gradle quickBuild. Your error should be fixed by running gradle ktlintFormat.

tomjschwanke · 2020-06-23T10:57:02Z

Thanks, I'm not used to CircleCI and style enforcement yet.
I have fixed it now and it runs quickBuild successfully

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/settings/SettingsTracingFragment.kt

harambasicluka · 2020-06-24T08:38:03Z

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/settings/SettingsTracingFragment.kt

@@ -101,6 +102,10 @@ class SettingsTracingFragment : Fragment(),
        switch.setOnClickListener {
            startStopTracing()
        }
+        // Additional click target to toggle switch
+        row.setOnClickListener {
+            startStopTracing()


the check has to be added here

…reSwitchBox

harambasicluka

Thanks 💯

fixes #455 by referencing the entire SettingsSwitchRow instead of jus…

7d5d5be

…t the SettingsSwitchRowSwitch

tomjschwanke requested a review from a team June 15, 2020 12:40

tomjschwanke changed the title ~~fixes #455 by referencing the entire SettingsSwitchRow instead of jus…~~ Make entire SettingsRow toggle the setting (closes #455) Jun 15, 2020

tomjschwanke mentioned this pull request Jun 16, 2020

[UX] Increase touch target for switches #455

Closed

harambasicluka self-assigned this Jun 17, 2020

harambasicluka added the ui Issue related to UI aspects label Jun 17, 2020

Fixed Switched having no OnClickListener anymore and therefore not wo…

5410944

…rking. Switches AND Rows now have an OnClickListener to change the setting.

tomjschwanke mentioned this pull request Jun 18, 2020

Fix for switches not working on slide (Fixes #607) #612

Merged

8 tasks

Merge branch 'dev' into fix/EntireSwitchBox

2f7387d

harambasicluka suggested changes Jun 22, 2020

View reviewed changes

Tom Jason Schwanke and others added 2 commits June 22, 2020 18:05

implemented requested changes from harambasicluka; (in onClickListene…

0dc17a1

…r) checking if row is enabled before executing action

Merge branch 'dev' into fix/EntireSwitchBox

e779e85

tomjschwanke requested a review from harambasicluka June 22, 2020 21:03

fixed formatting that caused quickBuild to fail

e764566

Merge branch 'dev' into fix/EntireSwitchBox

34d1a86

harambasicluka suggested changes Jun 24, 2020

View reviewed changes

Tom Jason Schwanke and others added 3 commits June 24, 2020 16:29

moved the isEnabled check to the row instead of switch

25e2239

Merge remote-tracking branch 'fork/fix/EntireSwitchBox' into fix/Enti…

2ebb8db

…reSwitchBox

Merge branch 'dev' into fix/EntireSwitchBox

6f07f43

harambasicluka approved these changes Jun 25, 2020

View reviewed changes

harambasicluka merged commit 39da305 into corona-warn-app:dev Jun 25, 2020

pwoessner mentioned this pull request Jul 20, 2020

Release 1.1.1 #893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make entire SettingsRow toggle the setting (closes #455) #466

Make entire SettingsRow toggle the setting (closes #455) #466

tomjschwanke commented Jun 15, 2020

tomjschwanke commented Jun 15, 2020

harambasicluka commented Jun 17, 2020 •

edited

Loading

tomjschwanke commented Jun 17, 2020 •

edited

Loading

tomjschwanke commented Jun 17, 2020 •

edited

Loading

tomjschwanke commented Jun 18, 2020 •

edited

Loading

tomjschwanke commented Jun 18, 2020 •

edited

Loading

harambasicluka Jun 22, 2020

tomjschwanke Jun 22, 2020

harambasicluka Jun 24, 2020

harambasicluka commented Jun 22, 2020

tomjschwanke commented Jun 22, 2020 •

edited

Loading

harambasicluka commented Jun 23, 2020

tomjschwanke commented Jun 23, 2020

harambasicluka Jun 24, 2020

harambasicluka left a comment

Make entire SettingsRow toggle the setting (closes #455) #466

Make entire SettingsRow toggle the setting (closes #455) #466

Conversation

tomjschwanke commented Jun 15, 2020

Checklist

Description

tomjschwanke commented Jun 15, 2020

harambasicluka commented Jun 17, 2020 • edited Loading

tomjschwanke commented Jun 17, 2020 • edited Loading

tomjschwanke commented Jun 17, 2020 • edited Loading

tomjschwanke commented Jun 18, 2020 • edited Loading

tomjschwanke commented Jun 18, 2020 • edited Loading

harambasicluka Jun 22, 2020

Choose a reason for hiding this comment

tomjschwanke Jun 22, 2020

Choose a reason for hiding this comment

harambasicluka Jun 24, 2020

Choose a reason for hiding this comment

harambasicluka commented Jun 22, 2020

tomjschwanke commented Jun 22, 2020 • edited Loading

harambasicluka commented Jun 23, 2020

tomjschwanke commented Jun 23, 2020

harambasicluka Jun 24, 2020

Choose a reason for hiding this comment

harambasicluka left a comment

Choose a reason for hiding this comment

harambasicluka commented Jun 17, 2020 •

edited

Loading

tomjschwanke commented Jun 17, 2020 •

edited

Loading

tomjschwanke commented Jun 17, 2020 •

edited

Loading

tomjschwanke commented Jun 18, 2020 •

edited

Loading

tomjschwanke commented Jun 18, 2020 •

edited

Loading

tomjschwanke commented Jun 22, 2020 •

edited

Loading