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

Add search/filter feature to the "Manage Sensor" screen #1430

Merged
merged 7 commits into from Mar 29, 2021
Merged

Add search/filter feature to the "Manage Sensor" screen #1430

merged 7 commits into from Mar 29, 2021

Conversation

chriss158
Copy link
Contributor

Summary

This PR adds the possibility to hide disabled sensors.

Screenshots

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

Additional changes:

  • Made "Disabled" sensor summary translatable
  • Show "Enabled" as sensor summary if sensor is enabled but has no state

@chriss158
Add setting "Show only enabled sensors"
b7b7144 
Additional changes:
* Made "Disabled" sensor summary translatable
* Show "Enabled" as sensor summary if sensor is enabled but has no state
dshokouhi
dshokouhi approved these changes Mar 11, 2021
View reviewed changes
Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Looks good to me, tested debug apk with a few sensors. Very minor comment from me since the string seems to be the same.

app/src/main/res/values/strings.xml Show resolved Hide resolved
@chriss158 chriss158 marked this pull request as draft March 27, 2021 20:12
@chriss158 chriss158 changed the title Add setting "Show only enabled sensors" Add search/filter feature to the "Manage Sensor" screen Mar 27, 2021
@chriss158
Copy link
Contributor Author

I reworked the PR. Mainly replaced the preference "Show enabled sensors only" by an actionbar item and added a search functionality in the actionbar.

@chriss158 chriss158 marked this pull request as ready for review March 27, 2021 23:15
@dshokouhi
Copy link
Member

I think the filter and search bar make it look clean :) great idea there!

JBassett
JBassett requested changes Mar 28, 2021
View reviewed changes
Copy link
Collaborator

@JBassett JBassett left a comment

Choose a reason for hiding this comment

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

Still not sold on storing the value but we can beta it.
Need to fix merge conflicts then good to merge.

@chriss158
Copy link
Contributor Author

No problem. Let's remove it and see if there are popping up issues. If yes, then we can still implement a setting 👍

@chriss158
Copy link
Contributor Author

I think i misunderstood you.
Storing a setting is only needed when the setting needs to be restored after the app was force closed. Right? That is not what i want.
I wanted that the setting is hold as long as the app runs. If the app was force closed or killed by the system, it's okay that the setting will be reset.

The showOnlyEnabledSensors is now a static class variable. It's value is hold as long the app is in the memory. Am i right here? Till now i thought the value is only hold while the Manage Sensors screen is open and will be reset after leaving the screen. But that is not the case 👍

@chriss158
Remove only_show_enabled_sensors preference
d794d1d
@chriss158
Merge remote-tracking branch 'upstream/master' into show-only-enabled…
202b87f 
…-sensors

# Conflicts:
#	app/src/main/java/io/homeassistant/companion/android/sensors/SensorsSettingsFragment.kt
#	app/src/main/res/values/strings.xml
@chriss158
Fix lint
94d7d84
@chriss158 chriss158 requested a review from JBassett March 28, 2021 19:22
JBassett
JBassett approved these changes Mar 29, 2021
View reviewed changes
Copy link
Collaborator

@JBassett JBassett left a comment

Choose a reason for hiding this comment

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

This was exactly what I was thinking!

@JBassett JBassett merged commit 31a48dd into home-assistant:master Mar 29, 2021
@dshokouhi dshokouhi mentioned this pull request Mar 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nick2525 nick2525 nick2525 left review comments

@dshokouhi dshokouhi dshokouhi approved these changes

@JBassett JBassett JBassett approved these changes

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@chriss158 @dshokouhi @nick2525 @JBassett @homeassistant