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

Add host placeholder to window and tab title formats #1570

Merged
merged 3 commits into from Jun 13, 2016

Conversation

Projects
None yet
4 participants
@djfinlay
Contributor

djfinlay commented Jun 10, 2016

This change is Reviewable

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 10, 2016

Current coverage is 81.45%

Merging #1570 into master will increase coverage by 0.03%

@@             master      #1570   diff @@
==========================================
  Files           106        106          
  Lines         15373      15369     -4   
  Methods           0          0          
  Messages          0          0          
  Branches       2466       2463     -3   
==========================================
+ Hits          12517      12519     +2   
+ Misses         2386       2382     -4   
+ Partials        470        468     -2   

Powered by Codecov. Last updated by 5c1401b...f9a1c8b

codecov-io commented Jun 10, 2016

Current coverage is 81.45%

Merging #1570 into master will increase coverage by 0.03%

@@             master      #1570   diff @@
==========================================
  Files           106        106          
  Lines         15373      15369     -4   
  Methods           0          0          
  Messages          0          0          
  Branches       2466       2463     -3   
==========================================
+ Hits          12517      12519     +2   
+ Misses         2386       2382     -4   
+ Partials        470        468     -2   

Powered by Codecov. Last updated by 5c1401b...f9a1c8b

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 10, 2016

Collaborator

Thank you! Looks good from a first quick look, but it might be Monday until I get around to merging it.

Collaborator

The-Compiler commented Jun 10, 2016

Thank you! Looks good from a first quick look, but it might be Monday until I get around to merging it.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 10, 2016

Collaborator

pylint detected a duplicate code snippet on Windows:

==qutebrowser.mainwindow.tabbedbrowser:175
==qutebrowser.mainwindow.tabwidget:114
        y = widget.scroll_pos[1]
        if y <= 0:
            scroll_pos = 'top'
        elif y >= 100:
            scroll_pos = 'bot'
        else:
            scroll_pos = '{:2}%'.format(y)

        fields['scroll_pos'] = scroll_pos
        try: (duplicate-code)

But I'm not sure how that's actually related to your change. Will investigate later.

Collaborator

The-Compiler commented Jun 10, 2016

pylint detected a duplicate code snippet on Windows:

==qutebrowser.mainwindow.tabbedbrowser:175
==qutebrowser.mainwindow.tabwidget:114
        y = widget.scroll_pos[1]
        if y <= 0:
            scroll_pos = 'top'
        elif y >= 100:
            scroll_pos = 'bot'
        else:
            scroll_pos = '{:2}%'.format(y)

        fields['scroll_pos'] = scroll_pos
        try: (duplicate-code)

But I'm not sure how that's actually related to your change. Will investigate later.

@djfinlay

This comment has been minimized.

Show comment
Hide comment
@djfinlay

djfinlay Jun 10, 2016

Contributor

I've refactored the methods for updating the tab and window titles as they were pretty similar.

Contributor

djfinlay commented Jun 10, 2016

I've refactored the methods for updating the tab and window titles as they were pretty similar.

@The-Compiler The-Compiler merged commit f9a1c8b into qutebrowser:master Jun 13, 2016

4 checks passed

codecov/patch 92.59% of diff hit (target 81.42%)
Details
codecov/project 81.45% (+0.03%) compared to 5c1401b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 13, 2016

Collaborator

Thanks! There also was the idea of using jinja2 for those settings a while ago, so you could expose url and do things like {{ url.host() }} in the setting instead.

Would you be interested to work on that by any chance? I opened a separate issue now, #1578.

Collaborator

The-Compiler commented Jun 13, 2016

Thanks! There also was the idea of using jinja2 for those settings a while ago, so you could expose url and do things like {{ url.host() }} in the setting instead.

Would you be interested to work on that by any chance? I opened a separate issue now, #1578.

@djfinlay

This comment has been minimized.

Show comment
Hide comment
@djfinlay

djfinlay Jun 13, 2016

Contributor

No problem. Sure, I'll take a look at it tonight.

Contributor

djfinlay commented Jun 13, 2016

No problem. Sure, I'll take a look at it tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment