nikintharan / openhab

Automatically exported from code.google.com/p/openhab
0 stars 0 forks source link

Pulseaudio Binding #234

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Hi Thomas,

did you already had a look at the binding. If it really should be part of the 
1.2 release there isn´t much more time for me to fix possible errors. Or 
should it be postponed to the next release?

Regards.
Tobias

Am Mittwoch, 3. April 2013 11:49:13 UTC+2 schrieb Thomas E.-E.:
no, no, Wiki is perfect!

- sent from a mobile device -

Am 03.04.2013 um 11:45 schrieb "Tobias Bräutigam" <tbrae...@gmail.com>:

No problem, I added the note to the Wiki page. Or did you wanted me to add the 
note somewhere else?

Am 03.04.2013 10:32, schrieb Thomas Eichstädt-Engelen:
very cool! Thank you!

Could you add a short note, that this binding will be available with the 
upcoming 1.2 release.

Regards,

Thomas E.-E.

On Apr 2, 2013, at 9:12 AM, Tobias Bräutigam <tbrae...@gmail.com> wrote:

Hi Thomas,

the Wiki page is online now. Maybe some parts of the explanation need some 
refinement, but the basiscs should be clear.

Regards
Tobias

Am 01.04.2013 22:45, schrieb Thomas Eichstädt-Engelen:
Hi Tobias,

did you started the Wiki-Page already? Shall i create the Start-Page for you?

Regards,

Thomas E.-E.

On Mar 27, 2013, at 10:55 AM, Tobias Bräutigam <tbrae...@gmail.com> wrote:

Np problem, I will prepare a wiki page meanwhile.

Am Mittwoch, 27. März 2013 10:48:38 UTC+1 schrieb Thomas E.-E.:
Hi,

i'll have a look at it. Could you prepare a wiki-page (like the others: 
http://code.google.com/p/openhab/wiki/Features > Bindings) meanwhile?

Thanks,

Thomas E.-E.

On Mar 27, 2013, at 10:45 AM, Tobias Bräutigam <tbrae...@gmail.com> wrote:

Well I think that my pulseaudio binding is ready to be tested as I am using it 
since months without problems. More information can be found here:
http://code.google.com/p/openhab/issues/detail?id=22

It is working for my use case, but as pulseaudio offers many possibilities, 
there may be other use cases which are not possible yet. But I guess the best 
way to find those problem is integrating it into the main repo and get more 
users to test it.

Original issue reported on code.google.com by teichsta on 10 Apr 2013 at 7:14

GoogleCodeExporter commented 8 years ago

Original comment by teichsta on 10 Apr 2013 at 7:15

GoogleCodeExporter commented 8 years ago
* all classes (especially those beneath internal.items) should contain (at 
least) class level javadoc
* all classes should contain @author tag
* all classes should contain @version 1.2.0 tag
* openhab_default.cfg should contain default parameter and comments (see mpd 
section)
* remove unused code (sys.out, etc.)
* remove Main.java

# Parser
* please document the various parse methods in more detail (javadoc)

# PulseaudioBindingProvider
* Item should not be used as return type since the Item itself is not really 
used here. It is more the items type which is relevant. It would be better to 
just return the type (Class<? extends Item> see HttpBindingProvider)

# PulseaudioBinding
* remove references (member, setter, getter) to EventPublisher since it is 
already contained in AbstractBinding superclass
* execute() and interncalReceiveCommand() are really looooooong methods. It 
would be good to split them to increase readability but better not before 1.2. 
Just put it on the TODO-list and refactor the code afterwards ;-).
## activate()
* decrease log level to debug (or remove logging here)
##deactivate()
* decrease log level to debug (or remove logging here)

# PulseaudioClient
* remove unused code
* move code from finalize() to explicit disconnect method and execute that 
method from PulseaudioBinding.deactivate()
* all Strings which cointains relevant "commands" should be extracted as static 
member and should be documented
* add more documentation

# PulseaudioGenericBindingProvider
* implement validateItemType()

# PulseaudioCommandTypeMapping
* remove unused code

# PulseaudioMonitor
* better letter PulseaudioBinding extends AbstractActiveBinding
* move execute code to PulseaudioBinding
* remove class
* remove pulseaudiomonitor.xml
* remove reference to pulseaudiomonitor.xml from MANIFEST.MF

After all: good work :-)

Thanks for this contriution!!

Original comment by teichsta on 10 Apr 2013 at 7:57

GoogleCodeExporter commented 8 years ago
Ok thank you for the hints. I have done all the suggested changes, except the 
following:

* all classes should contain @version 1.2.0 tag
Do you mean the @since 1.2.0 tag -> I have changed that, I did not see a 
@version tag in any other binding I looked into

* remove unused code (sys.out, etc.)
I did not found unused files like sys.out in my binding. All code that I had 
commentet out has been removed.

* execute() and interncalReceiveCommand() are really looooooong methods. It 
would be good to split them to increase readability but better not before 1.2. 
Just put it on the TODO-list and refactor the code afterwards ;-).
Well as you suggested I put this on my todo list after 1.2.0 release

I did i short test of the changed binding in my production system and 
everything seems to be ok, so from my point of view its ready to be merged.

Original comment by TBraeuti...@gmail.com on 10 Apr 2013 at 12:36

GoogleCodeExporter commented 8 years ago
wow - that's fast :-)

* yes, i meant the @since tag, sorry!
* by "sys.out" i meant "lines" like "//System.out.println("bla")", so 
everything is fine

Thanks a lot, will merge asap.

Regards,

Thomas E.-E.

Original comment by teichsta on 10 Apr 2013 at 12:43

GoogleCodeExporter commented 8 years ago
merged to default (see 
http://code.google.com/p/openhab/source/detail?r=692cbbbf5260b8b23218de252b81c81
2d797cbe2)

thanks again for this contribution!!

Original comment by teichsta on 10 Apr 2013 at 7:19