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

resolve issue #2797 #2808

Merged
merged 8 commits into from Jul 11, 2017
Merged

resolve issue #2797 #2808

merged 8 commits into from Jul 11, 2017

Conversation

@yashar-sb-sb
Copy link
Contributor

@yashar-sb-sb yashar-sb-sb commented Jul 10, 2017

Show messages longer if there are multiple of them.


This change is Reviewable

Copy link
Member

@The-Compiler The-Compiler left a comment

Can you please also add a test for this to tests/unit/mainwindow/test_messageview.py? You should be able to just show multiple messages, and then check view._clear_timer.interval().

multimpy_by = len(self._messages) + 1
if multimpy_by > 5:
multimpy_by = 5
interval *= multimpy_by

This comment has been minimized.

@The-Compiler

The-Compiler Jul 10, 2017
Member

You could simply do interval *= min(5, len(self._messages) + 1)

Why the + 1 though?

This comment has been minimized.

@yashar-sb-sb

yashar-sb-sb Jul 10, 2017
Author Contributor

Because new message will be added after starting the timer.
That was the way it was done in the original code. I didn't want to change the ordering.

This comment has been minimized.

@yashar-sb-sb

yashar-sb-sb Jul 10, 2017
Author Contributor

I can change The ordering and remove + 1.

This comment has been minimized.

@The-Compiler

The-Compiler Jul 10, 2017
Member

Ah, good catch! Yeah, I think it's safe to reorder it.

@yashar-sb-sb
Copy link
Contributor Author

@yashar-sb-sb yashar-sb-sb commented Jul 10, 2017

Okay I'll add the tests.

Add tests for "Show messages longer if there are multiple of them."
Check for interval being positive instead of checking for it to be
non-zero. So if somehow some unexpected thing happend and made
message-timeout negative, the bug doesn't cascade.
@The-Compiler The-Compiler merged commit 7da6908 into qutebrowser:master Jul 11, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 11, 2017

Thanks!

@yashar-sb-sb
Copy link
Contributor Author

@yashar-sb-sb yashar-sb-sb commented Jul 11, 2017

You're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants