Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Pass desktop name to muon and set/check xdg-settings default-web-browser #10974

Merged
merged 2 commits into from
Sep 18, 2017

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Sep 15, 2017

THIS PR MAKES brave/muon#309 REQUIRED BY #10552 LANDED SMOOTHLY

It is basically the subset of 5ec53ab

required by brave/muon#309
fix #1336

Auditors: @bsclifton, @bbondy

Test Plan:

  1. Set default browser to non Brave browser
  2. Open Brave from fresh profile
  3. You should see Brave ask to set as default
  4. Click Yes, in about:preferences#general, you should see Brave is your
    default browser
  5. Check system setting, default browser should be Brave

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@darkdh darkdh self-assigned this Sep 15, 2017
@darkdh darkdh requested review from bbondy and bsclifton September 15, 2017 21:03
@codecov-io
Copy link

codecov-io commented Sep 15, 2017

Codecov Report

Merging #10974 into master will increase coverage by 0.43%.
The diff coverage is 6.66%.

@@            Coverage Diff             @@
##           master   #10974      +/-   ##
==========================================
+ Coverage   54.15%   54.58%   +0.43%     
==========================================
  Files         249      249              
  Lines       21813    21835      +22     
  Branches     3395     3400       +5     
==========================================
+ Hits        11812    11918     +106     
+ Misses      10001     9917      -84
Flag Coverage Δ
#unittest 54.58% <6.66%> (+0.43%) ⬆️
Impacted Files Coverage Δ
js/stores/appStore.js 13.4% <6.66%> (-0.11%) ⬇️
...ponents/navigation/buttons/windowCaptionButtons.js 32.6% <0%> (-0.73%) ⬇️
app/common/lib/siteSuggestions.js 89.62% <0%> (-0.3%) ⬇️
app/common/lib/suggestion.js 68.46% <0%> (+0.11%) ⬆️
js/lib/urlutil.js 85.53% <0%> (+0.37%) ⬆️
js/stores/windowStore.js 29.45% <0%> (+0.86%) ⬆️
app/renderer/components/navigation/navigator.js 75.18% <0%> (+1.49%) ⬆️
js/state/frameStateUtil.js 58.89% <0%> (+4.61%) ⬆️
app/renderer/reducers/frameReducer.js 50.73% <0%> (+25.07%) ⬆️
... and 1 more

…wser`

requires brave/muon#309
fix #1336

Auditors: @bsclifton, @bbondy

Test Plan:
1. Set default browser to non Brave browser
2. Open Brave from fresh profile
3. You should see Brave ask to set as default
4. Click Yes, in about:preferences#general, you should see Brave is your
default browser
5. Check system setting, default browser should be Brave
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Ran into issues testing this...

  • I ran npm run build-package and npm run build-installer and then installed that RPM
  • When running Brave, I was able to click Set as Default on Settings > General. This did update the default web browser.
  • I set Always check on startup to true in Settings > General (for default browser)
  • I went into settings for Ubuntu, set default browser as Firefox
  • Exit Brave and relaunch
  • Try to set Brave as default browser
  • notice nothing happens

@bsclifton
Copy link
Member

Wait- disregard the above... the System Settings program doesn't update itself after the setting is changed. You have to close it and re-open it again 😛

That said- Brave does prompt each time (even though system settings shows it as the default)
screen shot 2017-09-18 at 1 29 42 pm

Maybe this is a different issue though, because setting it does work. What do you think, @darkdh?

(for xdg-settings which do not support `default-url-scheme-handler`)
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Tested with @darkdh on Ubuntu 14 (which had an old version of xdg-settings; v1.0.2). Updated logic (see second commit). Works great ++

@bsclifton bsclifton merged commit 2b120a0 into master Sep 18, 2017
@bsclifton bsclifton deleted the desktopNameArgLinux branch September 18, 2017 21:22
bsclifton added a commit that referenced this pull request Sep 18, 2017
Pass desktop name to muon and set/check `xdg-settings default-web-browser`
@bsclifton bsclifton added this to the 0.19.x (Beta Channel) milestone Sep 18, 2017
bsclifton added a commit that referenced this pull request Sep 18, 2017
Pass desktop name to muon and set/check `xdg-settings default-web-browser`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to set default browser on Ubuntu
4 participants