◐ Shell
reader mode source ↗
Skip to content

bpo-24241: Preferred X browser#85

Merged
ncoghlan merged 2 commits into
python:masterfrom
davesteele:preferred_browser
Feb 25, 2017
Merged

bpo-24241: Preferred X browser#85
ncoghlan merged 2 commits into
python:masterfrom
davesteele:preferred_browser

Conversation

@davesteele

Copy link
Copy Markdown
Contributor

When calling webbrowser.open*(), the module goes through a list of installed browsers, and uses the first one that succeeds, to process the request.

The first 'browsers' in the 'X' list are 'xdg-open' and others of that ilk. The problem is that they only have one 'open' behavior - the 'new' parameter is ignored ('same window', 'new window', 'new tab').

These commits move the default web browser to the front of the tryorder list, allowing get() behavior to match the documentation.

This same problem was recently fixed in Windows via Issue #8232

@the-knights-who-say-ni

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

  1. If you don't have an account on b.p.o, please create one
  2. Make sure your GitHub username is listed in "Your Details" at b.p.o
  3. If you have not already done so, please sign the PSF contributor agreement
  4. If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
  5. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

@davesteele davesteele changed the title Issue #24241: Preferred X browser Feb 14, 2017
@davesteele

Copy link
Copy Markdown
Contributor Author

I've tied my bpo 'daves' account to github 'davesteele'. The contributor form was received 2015-07-17.00:00:00.

@codecov

codecov Bot commented Feb 14, 2017

Copy link
Copy Markdown

Codecov Report

Merging #85 into master will decrease coverage by -0.01%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   82.38%   82.38%   -0.01%     
==========================================
  Files        1428     1428              
  Lines      351138   351143       +5     
==========================================
- Hits       289291   289275      -16     
- Misses      61847    61868      +21

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b692dc8...602ba9f. Read the comment docs.

@ultimatecoder ultimatecoder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Found improvements to the code.

@ultimatecoder ultimatecoder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

I will suggest updating the documentation for method register at Doc/library/webbrowser.rst

@davesteele

Copy link
Copy Markdown
Contributor Author

I've added documentation for the register() priority argument.

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

I like the general idea of this patch, but think it would be more self-explanatory with some changes to the specific implementation details.

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #85 into master will decrease coverage by -0.01%.
The diff coverage is 13.33%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   82.38%   82.37%   -0.01%     
==========================================
  Files        1428     1428              
  Lines      351138   351206      +68     
==========================================
+ Hits       289291   289317      +26     
- Misses      61847    61889      +42

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b692dc8...795ee09. Read the comment docs.

@davesteele

Copy link
Copy Markdown
Contributor Author

@ncoghlan, I've made some changes based on your review, and pushed back on a few others. You be the final arbiter on the issues. Let me know.

@ncoghlan

Copy link
Copy Markdown
Contributor

+1 on leaving the helper variable as a single string - I've suggested a name change and an additional explanatory comment to cover some of the subtleties for future maintainers.

For the preferred change, I still think keyword-only arguments will be clearer, as preferred=True conveys a lot more information at the call sites than a bare True.

The commit message also still needs adjustment to better cover the changes being made (i.e. both the preferred parameter being added to webbrowser.register, and the module being updated to use xdg-settings to find the appropriate preferred browser on Linux).

If you're so inclined, you could also add an entry to the 3.7 What's New for this change (otherwise I'll just add it along with the NEWS entry when pushing the change)

Replace the existing undocumented tri-state 'try_order' parameter with
the boolean keyword-only 'preferred' parameter. Setting it to True
places the browser at the front of the list, preferring it as the return
to a subsequent get() call.

'preferred' is added to the documentation.
The traditional first entry in the tryorder queue is xdg-open, which
doesn't support new window. Make the default browser first
in the try list, to properly support these options.
@davesteele

Copy link
Copy Markdown
Contributor Author

@ncoghlan, I've reworked, reworded, and rebased the commits based on your suggestions.

@ncoghlan

Copy link
Copy Markdown
Contributor

The codecov complaints are due to CI not running on Mac OS X, so this looks good to me now - merging!

@ncoghlan ncoghlan merged commit e3ce695 into python:master Feb 25, 2017
@davesteele

Copy link
Copy Markdown
Contributor Author

Thanks for your help on this.

@ncoghlan

Copy link
Copy Markdown
Contributor

Added the sprint label, as this PR was submitted at the PyCon Pune 2017 core development sprint.

akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017
…WatchdogEx

Fix issue python#85: don't schedule already scheduled tasklets.
Ensure that the internal watchdog list is empty before and after
all test cases of class TestNewWatchdog.

Fix test case TestNewWatchdog.test_watchdog_priority_{hard|soft}. The
timeout value was much to short to build up a list of watchdogs.

The new test cases TestNewWatchdog.test_schedule_deeper_{hard|soft}
triggers the assertion failure reliably. They are skipped for now.

https://bitbucket.org/stackless-dev/stackless/issues/85
(grafted from 2235702eb90cb0709dd40544b3952a956f2032a9 and 3fb55ce5b2d4)
akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017
akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017
colesbury referenced this pull request in colesbury/nogil Oct 6, 2021
Sometimes we need to acquire locks before the current PyThreadState
is set. This moves the waiter data to it's own struct. It's shared
between PyThreadStates on the same thread, such as in multi-interpreter
settings.

Fixes #85
jaraco pushed a commit that referenced this pull request Dec 2, 2022
markshannon pushed a commit to faster-cpython/cpython that referenced this pull request Jan 10, 2023
Add github workflow to build and publish docker image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants