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

Combine Garage and Rollershutter Components -> Cover #1949

Closed
n8henrie opened this issue Apr 30, 2016 · 16 comments
Closed

Combine Garage and Rollershutter Components -> Cover #1949

n8henrie opened this issue Apr 30, 2016 · 16 comments

Comments

@n8henrie
Copy link
Contributor

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:
- Rollershutter
- Garage Door

Description of problem:

Relevant discussion: home-assistant/frontend#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
Copy link
Member

balloob commented May 2, 2016

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 ?

@sfam
Copy link
Contributor

sfam commented May 2, 2016

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
#1949 (comment)

@n8henrie
Copy link
Contributor Author

n8henrie commented May 2, 2016

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
Copy link
Member

balloob commented May 3, 2016

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
Copy link
Member

fabaff commented Jun 21, 2016

@fabaff
Copy link
Member

fabaff commented Jun 24, 2016

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

@n8henrie
Copy link
Contributor Author

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.

  • I was hoping to help move over some of the existing platforms, but I don't have e.g. a Wink Garage Door 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 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).

@philk
Copy link
Contributor

philk commented Jun 24, 2016

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
home-assistant/frontend#54 (comment)
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
    disabled state for buttons was reversed frontend#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
#1949 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABzerhn0uP467CXGWSPk39VRT2ED_Heks5qO_ZKgaJpZM4ITcf6
.

@n8henrie
Copy link
Contributor Author

n8henrie commented Jun 24, 2016

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
Copy link
Contributor

philk commented Jun 24, 2016

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
Copy link
Member

balloob commented Jun 26, 2016

@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
Copy link

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

@n8henrie
Copy link
Contributor Author

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 added a commit to n8henrie/home-assistant that referenced this issue Jun 29, 2016
Make a first few steps towards merging garage_door and rollershutter
(home-assistant#1949) by combining their basic
__init__.py files, demo.py, and changing all the class / function names
and docstrings.

No major changes in function from prior, just getting started by
changing the names.
n8henrie added a commit to n8henrie/home-assistant-polymer that referenced this issue Jun 29, 2016
- Fix service names: `open` instead of `open_cover`
- Reverse previous definition of open / close:
  - home-assistant/core#1941
  - home-assistant/core#1949
  - home-assistant#54
  - home-assistant#58
@n8henrie
Copy link
Contributor Author

n8henrie commented Jun 29, 2016

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
Copy link
Member

fabaff commented Jul 2, 2016

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
Copy link
Contributor

Closed via #2891

@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants