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.

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

Don't hide location entities that are "home" in the MapViewStrategy #23462

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

Hypfer
Copy link
Contributor

@Hypfer Hypfer commented Dec 26, 2024

Proposed change

I've discovered that the MapViewStrategy that populates the default map dashboard filters any entity that is "home" no matter if it has geolocation data. Checking the history, I can see that this is how it always worked:
https://github.com/home-assistant/frontend/blame/7542c03dbbfd64e20dbc061ab6d271d4aec8a76a/src/panels/map/ha-panel-map.ts#L61-L87

I'd like to challenge that, because to me, it is very confusing.

I first thought that maybe my android companion app was misconfigured, because even though I enabled my location, I just couldn't see anything. There was simply no feedback excluding maybe the devtools where my person entity suddenly gained latitude/longitude attributes.
I was considering that maybe I'm supposed to "Take Control" of the Map dashboard, however that also seemed wrong so I started digging and eventually found that filter.

Another inconsistency about this filter is that the person marker will show up while in any other configured zone (e.g. work, school, etc.) but not while home.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue) - arguably a UX bugfix but might also just be an opinion
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Default config

Additional information

  • This PR fixes or closes issue: fixes N/A
  • This PR is related to issue or discussion: N/A
  • Link to documentation pull request: N/A

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@MindFreeze MindFreeze added the Needs UX Pull requests requiring a review from the Home Assistant design team label Dec 27, 2024
@Hypfer
Copy link
Contributor Author

Hypfer commented Dec 30, 2024

This other PR I opened could also be considered related, as - when there are multiple gps and non-gps device_trackers configured - it greatly increases the likelihood that a person entity that is "home" has geo-coordinates in the first place:
home-assistant/core#134075

Probably also something best to be reviewed by the UX Team

@MindFreeze
Copy link
Contributor

We discussed it with the UX team and agreed that this should be an option to the map strategy. Changing it for everyone would be too disruptive.

@marcinbauer85
Copy link
Contributor

@Hypfer We've prepared a design that shows how this could be done without changing existing behaviors.
Please see the below design for all notes, and feel free to ask questions. Generaly this is to be done similarly like the current "Defautl dashboard" editing experience is, as in when you create or edit it, you have additional options just for that dashboard (strategy).

https://www.figma.com/design/pJzweI4KABKhrDzNwNTVn5/PDX-1911-Fix-not-hiding-'at-home'-people-in-map-strategy?node-id=0-1&p=f&t=KPhUaTjdNkh9Qn8J-11

@Hypfer
Copy link
Contributor Author

Hypfer commented Jan 20, 2025

@marcinbauer85 While I do appreciate that you people put time into considering this, I must say that this is quite a steep ask, given that I'm not on your payroll.

Specifically communication like this is what I'd consider the paid kind of communication:

Please see the below design for all notes, and feel free to ask questions.

Emotionally, I find that a bit disrespectful and value-extraction-y.
Factually, I think it lacks awareness and understanding of the different roles and hats we wear + where we're coming from.


To shed some light onto my situation:

I have an automated process that makes it trivial for me to run my custom patched home assistant instance. Because of that, there is very little pain for me that would encourage me to upstream what I do.
I did decide to try so regardless, because it is objectively correct to do so. Someone needs to care.

That said, I committed to upstreaming a single line change.
That was rejected and I can understand why, but just because I committed to upstream a single-line-patch, doesn't mean that I also committed to build a whole feature based on some figma design, because that is what contractors do.


What I'd be at the very least expecting would be an acknowledgement that the scope increased by at least 10x, that you don't expect me to do it, but if I'd care then thank me and here's where to look.
You don't even have to mean it, but the communication for sure lacks the business-style sugarcoating that is necessary to not alienate people when doing business-style task assignment, because as said, I'm not a nabu casa employee nor paid by it as a contractor.

Granted, I suppose it's not always necessary and people will comply regardless, but at this point I feel that I personally contributed enough to the ecosystem to deserve that kind of acknowledgement and treatment as a human.
I'd want to argue that everyone should deserve that regardless of contribution, but I'm also aware of the dreadful state that is the smarthome foss space, so I'm trying to keep it realistic.

@ildar170975
Copy link
Contributor

@marcinbauer85
Your figma design relates to an automatically created dashboard based on on strategy. Assume you are proposing an option “show persons at home” as a part of this strategy.

As for a stock Map card - you will need to provide a similar option because currently entities w/o coordinates are not shown.
And same then will have to be done for any custom map cards.
Imho adding coordinates for a “person” entity which is at home would be a reasonable improvement.

@marcinbauer85
Copy link
Contributor

marcinbauer85 commented Jan 21, 2025

@Hypfer All your contributions are greatly appreciated, and I had no intentions to force you to complete this as per attached design. Frankly I'm surprised how you could have thought that since it's even you who noted that the design team should chime in - hence here I am.
As you mentioned FOSS contribution is entirely up for the contributor to decide what and where to do, and if this is something that you don't want to pursue right now, we can leave it at that and close this PR. The design will be posted as a help-wanted post on the discussion for someone to pick up if available.

@marcinbauer85
Copy link
Contributor

As for a stock Map card - you will need to provide a similar option because currently entities w/o coordinates are not shown.
And same then will have to be done for any custom map cards.
Imho adding coordinates for a “person” entity which is at home would be a reasonable improvement.

@ildar170975 Are you certain about this? Entities from the person domain have lat and long if they have a device_tracker entity added to them via the People UI. I've successfully added person.* (that has a device_tracker) or device_tracker.* to the Map card and they are displayed in the Map card in the Home zone.

@ildar170975
Copy link
Contributor

ildar170975 commented Jan 21, 2025

@marcinbauer85
Assume a person has two device tracker entities associated (gps and router based). At home a person has no coordinates.
I am far from a normal PC these days, cannot test right now, but this is how it worked recently.

@Hypfer
Copy link
Contributor Author

Hypfer commented Jan 21, 2025

@marcinbauer85 Thank you for your reply.
With the added context, I feel like we might both have been lost in the command chain/process/communication here.

Indeed I said that it might make sense for the UX team to have a look. I did not expect that to become a To-Do item though. But it probably did. Probably, there's some internal ticketing system and that maybe way assigned to you?
Something like that could be plausible.

I think what makes this really hard is how the lines between commercial software project and open source kinda blur. We have the FOSS-style contribution at the same time with what is for some people just their day job.
We also have a bunch of internal processes, documentation, structures etc that are opaque to outside people like me but do control very much of how it actually works.

Communication troubles might also just be growing pains. Who's to say.
Anyway, not complaining, just interested in resolving that :)

Speaking of resolving.
First of all, again, thank you for taking your time to come up with the design.
I like the approach and that you've considered the various perspectives on that, I did not see when initially proposing the PR.

Personally, I'm a person that really doesn't click with highly compartmentalized and standardized structures and processes of large commercial entities, so if someone throws a figma design at me and says "hey, do it like this", my initial reaction is to shriek.
Then again, I am by no means the norm and other contributing people would usually be very happy about being given such strong guidance and not having to figure things out on their own.

Another thing that is mentally holding me back here is that I still haven't gotten around to setting up the dev setup, because when I did, it requested full access to my GitHub account; so I shrieked and threw it away.
That however is completely unrelated to anything UX. Just a personal blocker for me

That all said, I'll see if I can overcome those two things and implement the design.
It does look a bit daunting because it seems foundational, but it also looks like whatever comes out of it might be useful in the future as well, so definitely the right call to do it like this.

@Hypfer
Copy link
Contributor Author

Hypfer commented Jan 21, 2025

@ildar170975

I think you might be mixing things up. While those topics are both things I proposed and called "somewhat relevant to each other", what you're after is happening in a different PR. Specifically home-assistant/core#134075

@marcinbauer85
Copy link
Contributor

marcinbauer85 commented Jan 21, 2025

Indeed I said that it might make sense for the UX team to have a look. I did not expect that to become a To-Do item though. But it probably did. Probably, there's some internal ticketing system and that maybe way assigned to you?
Something like that could be plausible.

Yes, designers at Nabu Casa do monitor PR's for feedback or help with figuring out from a grand scale view (and having internal insights) what to decide in some aspects of HA that are aligned with what the community wants, and what it expects.
Github has has been and still is to a greater degree an environment for developers. I've joined Nabu Casa in late last year, and I'm helping to bridge the gap and by publishing open design from Figma - be more transparent from the design side.

I'm glad we've cleared this up. Let's focus on how we can move forward, and if you have any other questions you can always message me at discord @esseti85 💪

@ildar170975
Copy link
Contributor

@Hypfer
Check this post: #23462 (comment)
Here I just speculated about a possible similar change in a stock Map card.

@marcinbauer85
Copy link
Contributor

@Hypfer could you update your current status on this?

@Hypfer
Copy link
Contributor Author

Hypfer commented Jan 21, 2025

@marcinbauer85 Uh.. I'm not sure if I understand the question correctly.

My status is the same as 6h ago. As said, I'm just a random guy that is not on nabu casas payroll.
I am on other people's payroll though. Hence, in those last 6 hours I did earn that paycheck.

I'm a bit confused why there should have been a change within that timespan, given that this work would if it would happen be purely volunteer and free-time work.
That kind of work don't really come with deadlines and definitely not with end-of-business-day deadlines 👀

@MindFreeze
Copy link
Contributor

@Hypfer sorry if it seemed pushy. The reason, Marcin asked is because we can take responsibility for the additional changes. As you said, they are beyond your initial intentions. If you want to continue working on this, please let us know. We would be grateful, of course. Otherwise we can merge this as is and continue with additional enhancements in a separate PR.

@Hypfer
Copy link
Contributor Author

Hypfer commented Jan 21, 2025

@MindFreeze Ah I see! Thanks for the clarification.

Yeah, in that case, I don't want to block this. Feel free to take over.
Earliest I could look at this would be in 3 weeks and that's already a big maybe. There is just so much stuff going on 🫠

Thanks!

Copy link
Contributor

@MindFreeze MindFreeze left a comment

Choose a reason for hiding this comment

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

TBH, we may not be able to get to it sooner but we're trying to keep open PRs to a minimum.
Thank you for the contribution!

@MindFreeze MindFreeze merged commit eaab19f into home-assistant:dev Jan 21, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Needs UX Pull requests requiring a review from the Home Assistant design team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants