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

Support for ChangeListener #31

Merged
merged 1 commit into from
Oct 24, 2015
Merged

Support for ChangeListener #31

merged 1 commit into from
Oct 24, 2015

Conversation

Petikoch
Copy link
Contributor

Hi @mikebaum,

I implemented support for ChangeListener's for: JTabbedPane, JSlider, JSpinner, SpinnerModel, AbstractButton, ButtonModel and JViewport. Would be nice to have that soon in RxSwing.

About the implementation:
Since there is no "common" interface for the two common methods addChangeListener and removeChangeListener of classes like JTabbedPane, JSlider, etc... I decided to use internally reflection to avoid duplicated code. Using reflection gives a little performance penalty, but in the seldom case of registering / unregistering change listeners, this should not be an issue.

I know this PR is competing the work and the two PR's from @samuelgruetter. It's based on the typical style of RxSwing and does not introduce refactorings, but comes "ready to use" with tests. So it's a quick solution to have support for ChangeListener now, without considering design improvements. What do you @samuelgruetter and @mikebaum think?

Best regards from Lucerne in Switzerland,
Peti

@Petikoch
Copy link
Contributor Author

Opps... just saw that there is another issue regarding that topic in #27 from @whymarrh.

@mikebaum
Copy link
Collaborator

@Petikoch @samuelgruetter
I think there are elements from both these pull requests #27 that make sense. However since this pull request is complete and I don't have a lot of time to work on the one started by @samuelgruetter I will review this one.
What I like about #27 is the addition of a generalization of the creation of an event stream from an event source. I think that could be a PR in and of itself and we could refactor all event sources to use this pattern. It removes a lot of code duplication. Would be even better with lambdas, but since we're bound to java 6 it's not worth discussing.

@Petikoch
Copy link
Contributor Author

Hi @mikebaum, thanks for pointing me to https://docs.oracle.com/javase/tutorial/uiswing/events/eventsandcomponents.html (didn't know it). I just updated the PR. Best regards, Peti

@mikebaum
Copy link
Collaborator

Okay I finished my review. After addressing the comments, perhaps you can squash all your commits into one patch.

@Petikoch
Copy link
Contributor Author

Thanks a lot @mikebaum for the review. I'll update the PR tonight or tomorrow. Best regards, Peti

@mikebaum
Copy link
Collaborator

@Petikoch Sounds good, thanks for the PR.

@Petikoch
Copy link
Contributor Author

@mikebaum, the PR is updated.

Method method = object.getClass().getMethod(methodName, parameterTypes);
if (!Modifier.isPublic(method.getModifiers())) {
throw new IllegalArgumentException(
"'" + object + "' has not the expected signature to support change listeners in "
Copy link
Collaborator

Choose a reason for hiding this comment

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

This message relies on the object having a sane implementation of toString(), perhaps, it would be more informative to rather to use object.getClass().getSimpleName(). This comment applies to the other instances within this file of using object in a message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I updated the PR.

@mikebaum
Copy link
Collaborator

Okay I finished my review. Just a few minor things to change. Once you make those updates, please squash all the commits into one commit and I'll merge it.

👍

Although I'm not a big fan of reflection, in this case, for the reasons you stated it is a reasonable compromise. We could potentially generalize and use this approach if we discover other event sources that do not share a common interface. It's a shame we don't have java 8, because then we could have used method references rather than reflection.

@mikebaum
Copy link
Collaborator

@Petikoch So everything seems fine... Can you just squash the 5 commits into one. Not sure if you've done that before on github, but it works and magically updates the PR. Here's a link that describes what to do. There are probably others.

@Petikoch
Copy link
Contributor Author

Hi @mikebaum, I finally managed to squash all changes into one commit and update my feature branch :-)

@Petikoch
Copy link
Contributor Author

@mikebaum, after sleeping one night... the following idea came up... what do you think about putting just one fromChangeEvents method with a single parameter of type Object into SwingObservable? I think this would be better.

@mikebaum
Copy link
Collaborator

@Petikoch I think it's okay the way it is. This way the users of SwingObserverable don't have to search which classes support ChangeListeners.

mikebaum added a commit that referenced this pull request Oct 24, 2015
@mikebaum mikebaum merged commit c30edde into ReactiveX:0.x Oct 24, 2015
@mikebaum
Copy link
Collaborator

@Petikoch merged, good work!

@Petikoch
Copy link
Contributor Author

@mikebaum thanks again a lot for the review!

@kittinunf
Copy link

Could we have a release that contains this 0.26 maybe?

@akarnokd
Copy link
Member

akarnokd commented Aug 9, 2016

This library doesn't seem to be under active development. It is setup to support one-button release though. Let me see if I can get a release out but the version-tool doesn't seem to like 0.x version patterns.

@akarnokd
Copy link
Member

akarnokd commented Aug 9, 2016

@kittinunf I've released 0.26.0 without problems. It may take some hours to show up on maven central.

@Petikoch
Copy link
Contributor Author

Petikoch commented Aug 9, 2016

Thanks @akarnokd for releasing!

@mikebaum
Copy link
Collaborator

@akarnokd Thanks for releasing that update. Just wondering where is the "release" button? I would have released it long ago, but alas life distracted me.

@Petikoch poke

@kittinunf @Petikoch I have outstanding PRs on this project, would one of you like to review them. Also, I laid out a bunch of issues that I consider the minimum to release a v.1 of this project. It's not particularly challenging code, but necessary. Would any of you be interested in spending some hours finish this up? I'll work on it too, I could knock out a few of them quickly. I'll send you all a virtual beer!

@akarnokd
Copy link
Member

There is a release tab on GitHub where if you create a new release with tag vx.y.z, the CI will perform the release automatically.

@kittinunf
Copy link

Sorry for being late to the party. Thanks @akarnokd for releasing the 0.26.0, it works like a charm.

@mikebaum I am not sure I am qualified enough as I am writing Kotlin a lot these days. I'll do what I can do.

@Petikoch Petikoch deleted the supportChangeListener branch August 26, 2016 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants