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

qtutils: use the proper clicked signal instead of pressed #715

Merged
merged 1 commit into from Aug 1, 2017

Conversation

Projects
None yet
2 participants
@jeff-dagenais
Copy link

jeff-dagenais commented Jul 27, 2017

Reacting to the pushed event makes the interface behave unlike any other
GUI out there. Restore the normal clicked behaviour. Fix #604 using a
bounce function.

This reverts d9a3f3f while still
fixing #604

Jean-Francois Dagenais
qtutils: use the proper clicked signal instead of pressed
Reacting to the pushed event makes the interface behave unlike any other
GUI out there. Restore the normal clicked behaviour. Fix #604 using a
bounce function.

This reverts d9a3f3f while still
fixing #604
@davvid
Copy link
Member

davvid left a comment

There's actually a very good reason why we use clicked, not that we can't use pressed, but what this MR does is insufficient. The use of pressed was a pragmatic choice. It was the simplest because its signature has not changed between Qt 4 and 5.

This MR breaks multi-version Qt compat where the clicked(pressed=bool) callback does not always provides the pressed bool argument to the function.

In older Qt4, the argument was not provided unless you explicitly .clicked[bool].connect(...) to the signal, and the bareword one gives you a callback that doesn't include the bool argument.

In Qt5, the bool argument is provided when using the bareword signal, so the code needs to accommodate both signatures.

The full solution is to change this is to add a def wrapper(*args, **kwargs) wrapper inside connect_button() that consumes the arguments, calls the fn(), and then attaches that wrapper to the signal instead of the user-provided function.

Then you don't need the "takes care of argument mismatch" stuff, and it makes sure that we're not breaking dozens of other callsites that may or may not be prepared to deal with a change in signature.

@jeff-dagenais

This comment has been minimized.

Copy link

jeff-dagenais commented Jul 28, 2017

Sorry for the naive patch.

  1. I am no python champion yet,
  2. Although I have been using cola for 6-7 years, I have very little knowledge in the source code.

What I do know is that using GUI apps which react immediately on button pressed event hurts the brain. Feels like my mouse is broken... or is it my brain...

I definitely think we should not sacrifice the quality of user experience for a simple API signature change.

So with the provided suggestion about the def wrapper... I can give it a whirl and re-submit.

@davvid

This comment has been minimized.

Copy link
Member

davvid commented Jul 29, 2017

Sweet, thanks for the careful attention to these usability issues, I believe these changes will be well received all around, and wholly agree that user experience is more important than code simplicity.

In case it helps, here's roughly what I was describing (I guess this is where I should have added a comment about the previous choice)

def connect_button(self, button, fn):
    def wrapper(*args, **kwargs):
        """Consume clicked's callback arguments to handle differences in behavior between Qt 4 and 5"""
        fn()
    button.clicked.connect(wrapper)

Looking forward to it, and thanks again!

davvid added a commit to davvid/git-cola that referenced this pull request Aug 1, 2017

Merge pull request git-cola#715 from jeff-dagenais/restore_normal_but…
…ton_behaviour

* jeff-dagenais/restore_normal_button_behaviour:
  qtutils: use the proper clicked signal instead of pressed

Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Aug 1, 2017

qtutils: handle all supported versions of Qt
Add a lambda to paper over the signature change between Qt 4/5.  The
callback at "button.clicked" changed from "clicked()" to "clicked(bool)"
so accommodate all versions adding a lambda wrapper that consumes all
callback-provided arguments.

Related-to: git-cola#715
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Aug 1, 2017

doc/relnotes/v3.0: add notes about button behavior in git-cola#715
Signed-off-by: David Aguilar <davvid@gmail.com>

@davvid davvid merged commit ef4693a into git-cola:master Aug 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment