search

home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
73.7k stars 30.82k forks source link

Combine Garage and Rollershutter Components -> Cover #1949

Closed n8henrie closed 8 years ago

n8henrie commented 8 years ago

Make sure you run the latest version before reporting an issue. Feature requests should go in the forum: https://community.home-assistant.io/c/feature-requests

Component/platform:

Description of problem:

Relevant discussion: https://github.com/home-assistant/home-assistant-polymer/pull/54

The Garage Door and Rollershutter components serve basically the same major function and share substantial portions of their codebase. They could conceivably be combined into a single component to eliminate redundant code and simplify component maintenance for the future. @balloob has suggested the component name cover, which is seems sufficiently generic (as this will likely be used for garage doors, overhead doors, certain times of blinds, window rollershutters, etc.), and shorter and easier to spell than rollershutter.

I'm going to try to work on this over the next little while but wanted to open an issue for relevant discussion and feedback, as I'm not a very experienced Pythonista.

balloob commented 8 years ago

For garage doors we currently have the methods is_closed, close_door, open_door. Rollershutter has is_open, move_up, move_down, stop and can expose current position.

The rollershutter allows to stop mid-movement, but not all cover implementations will support this.

sfam commented 8 years ago

We attemped this before releasing rollershutter... We even had the name motor for it... then we decided to keep this separate, because they will be more user friendly that way... no one will ever google "how to automate my cover"...

Paulus Schoutsen notifications@github.com escreveu no dia segunda, 2/05/2016 às 07:49:

For garage doors we currently have the methods is_closed, close_door, open_door. Rollershutter has is_open, move_up, move_down, stop and can expose current position.

The rollershutter allows to stop mid-movement, but not all cover implementations will support this.

  • can stop
  • is fully open
  • is fully closed
  • position
  • in motion ?

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/home-assistant/home-assistant/issues/1949#issuecomment-216117836

n8henrie commented 8 years ago

I agree, and that was one of my concerns as well. However, most of the platform pages aren't what people need to find anyway, and as long as the component pages also use the word garage it should still have decent ranking I would think. Especially the more "plug and play" solutions (e.g. Wink) would have the component page have the phrase garage door all over it.

I think a bigger issue than how people arrive from outside search engines is how to arrive from the main Components page -- I've browsed through this dozens of times, and as more and more components are added, it gets tougher to navigate to each one to figure out what it does if its name is at all vague / ambiguous.

balloob commented 8 years ago

SEO should not impact how we pick our names. That's something for organization of our site.

@sfam I know we've had these discussions before but it was only recently that via an opened issue I realized that open/close is not very straightforward. And if we flip it, well then it becomes identical to a garage door so let's make them 1.

fabaff commented 8 years ago

Story: https://www.pivotaltracker.com/story/show/118556451

fabaff commented 8 years ago

@n8henrie any plans to create a PR with your work?

n8henrie commented 8 years ago

Thanks for the nudge.

Yes, but as I mentioned in the other thread this is my first hass contribution and my first larger open source project contribution, so I'm unclear on a few points.

philk commented 8 years ago

I'd be happy to test your changes for the Wink blinds (since I wrote that component :). Can't help you with the garage door though.

I definitely see open as open if they're called a cover. Fine with it being reversible but it'd be maddening to have to "close" my blinds to open them. On Fri, Jun 24, 2016 at 8:22 AM Nathan Henrie notifications@github.com wrote:

Thanks for the nudge.

Yes, but as I mentioned in the other thread https://github.com/home-assistant/home-assistant-polymer/pull/54#issuecomment-215978317 this is my first hass contribution and my first larger open source project contribution, so I'm unclear on a few points.

  • I was hoping to help move over some of the existing platforms, but I don't have e.g. a Wink Garage Door https://home-assistant.io/components/garage_door.wink/ to make sure I've ported the code correctly / for debugging. In fact, the only one I have is a homebrewed command line rollershutter.
    • I've already gone through and changed the variable names and docstrings e.g. from garage_door and rollershutter to cover
    • Will obviously need testing by someone with access to the corresponding devices before these new versions should go live
  • At what point will the old garage_door and rollershutter components be removed? Perhaps these should stay for a while with some kind of deprecation notice?
  • For reverse compatibility, I plan to leave the existing behavior that sparked this conversation https://github.com/home-assistant/home-assistant-polymer/pull/54 as default -- i.e. an "open" cover is what I think of as e.g. a "closed" garage door (an unrolled rollershutter) -- but implement an option to reverse this to fit better with what some of us think is more intuitive behavior (so one would "open" a cover in order to walk through it and close it to secure it).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/home-assistant/home-assistant/issues/1949#issuecomment-228375439, or mute the thread https://github.com/notifications/unsubscribe/AABzerhn0uP467CXGWSPk39VRT2ED_Heks5qO_ZKgaJpZM4ITcf6 .

n8henrie commented 8 years ago

but it'd be maddening to have to "close" my blinds to open them.

@philk This is the current situation, correct?

Are you saying you currently don't have the open and close reversed? (Currently position 0 == rollershutter / garage is closed == door / window is open)

philk commented 8 years ago

Good question I guess. The blinds handle move_up and move_down and at least on Wink they don't expose status so I guess I don't know if they're "open" or "closed".

balloob commented 8 years ago

@n8henrie Just copy all the platforms over. Let PyLint figure out the most obviously mistakes. We'll release both cover together with garage door/rollershutter for 2 releases (4 weeks) and after that will remove the old ones. That will give us time to make sure it all is up and running.

Looking forward to a PR.

faljeremy commented 8 years ago

guys how can i use scene for my rollershutters? move_up? upcmd? just up?

n8henrie commented 8 years ago

Hoping to send out the PR this afternoon, sorry for delays -- had some trouble getting the frontend dev environment set up.

Settled on using the e.g. open_cover terminology instead of move_up, hope that's okay.

n8henrie commented 8 years ago

In the HA pull request template, I see a footnotes link to a post on another site about squashing commits -- it doesn't appear in the rendered version of the page. Do I need to squash before I make a PR?

If so, could I make this more explicit in either the PR template or in CONTRIBUTING.md?

fabaff commented 8 years ago

In the HA pull request template, I see a footnotes link to a post on another site about squashing commits -- it doesn't appear in the rendered version of the page. Do I need to squash before I make a PR?

We had this before Github offered to do the squashing while merging.

turbokongen commented 8 years ago

Closed via #2891