Ensuring File > Share context menu items are disabled when no windows are open #13933

ryanml · 2018-04-25T01:55:55Z

This PR also includes some refactoring for the menu code concerning setting the checked status of a menu item. The functions are now generalized to setting a given attribute, so that enabled can be toggled as well.

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.
Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Launch Brave from the terminal.
Close any open windows, but ensure Brave is still running.
Select File > Share
Ensure that all of the submenu options are disabled
Ensure that no error is being shown in the terminal
Open a new window
Select File > Share
Ensure that the submenu options are enabled again

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

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

codecov-io · 2018-04-25T02:15:57Z

Codecov Report

Merging #13933 into master will decrease coverage by <.01%.
The diff coverage is 60.71%.

@@            Coverage Diff             @@
##           master   #13933      +/-   ##
==========================================
- Coverage   56.46%   56.45%   -0.01%     
==========================================
  Files         284      284              
  Lines       29340    29362      +22     
  Branches     4872     4877       +5     
==========================================
+ Hits        16566    16577      +11     
- Misses      12774    12785      +11

Flag	Coverage Δ
#unittest	`56.45% <60.71%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
app/common/commonMenu.js	`53.24% <100%> (+0.3%)`	⬆️
app/common/lib/menuUtil.js	`71.07% <100%> (+0.28%)`	⬆️
app/browser/menu.js	`51.69% <47.61%> (-0.47%)`	⬇️

bsclifton · 2018-05-01T16:25:27Z

app/browser/menu.js

+ case appConstants.APP_WINDOW_CREATED:
+ {
+ const windowCount = BrowserWindow.getAllWindows().filter((w) => w.isFocused()).length
+ if (action.actionType === 'app-window-closed' && windowCount === 0) {


could you use the constant name here instead?
if (action.actionType === appConstants.APP_WINDOW_CLOSED && windowCount === 0) {

bsclifton

comments left- otherwise, works and looks great! 😄 👍

bsclifton · 2018-05-01T16:25:41Z

app/browser/menu.js

+ const windowCount = BrowserWindow.getAllWindows().filter((w) => w.isFocused()).length
+ if (action.actionType === 'app-window-closed' && windowCount === 0) {
+ updateShareMenuItems(state, false)
+ } else if (action.actionType === 'app-window-created' && windowCount === 1) {


Same as above comment

petemill

Changes requested to make sure we are using window API that considers tabbed browser windows instead of any muon/electron window

petemill · 2018-05-01T16:57:08Z

app/browser/menu.js

+ case appConstants.APP_WINDOW_CLOSED:
+ case appConstants.APP_WINDOW_CREATED:
+ {
+ const windowCount = BrowserWindow.getAllWindows().filter((w) => w.isFocused()).length


Please don't use the muon BrowserWindow API raw. Instead please use the app/browser/windows.js API, specifically getAllRendererWindows or getActiveWindow. The former should be enough as it makes sure the hidden window isn't included.

Good call- I was wondering that myself 😄

ryanml · 2018-05-01T19:11:27Z

Changes pushed

cc: @bsclifton @petemill

bsclifton

Works great! Updated changes look good too

petemill

Works nicely 👍 . Just left a comment about the neccessity of excludeShareSites rather that just removing the digg entry as I don't think it's being used anywhere else. Worth another search in order to reduce the complexity. 🔍

petemill · 2018-05-01T21:44:38Z

app/browser/share.js

@@ -19,6 +19,11 @@ const templateUrls = {
 reddit: 'https://reddit.com/submit?url={url}&title={title}'
 }

+// Sites that are not currently in the File > Share context menu
+const excludedShareSites = [


Do we need this rather than just remove the digg item from templateUrls. From a quick search I could not find any other usage of the templateUrls export from share.js.

@petemill good thought, I did a search and it looks fine to remove. I've pushed an update removing the excludedShareSites addition. I left removing Digg from templateUrls a separate commit for ease of search should we want to add it back at any point.

Refactoring menu.js item attribute functions

petemill

LGTM, thanks for the additional fixes and cleanup 🥇

Ensuring File > Share context menu items are disabled when no windows are open

bsclifton · 2018-05-01T22:46:29Z

master ca83ee3
0.23.x 1954b5c
0.22.x-release3 4403af2

ryanml requested review from petemill and LaurenWags April 25, 2018 01:56

ryanml added refactoring feature/context-menu labels Apr 25, 2018

ryanml force-pushed the file-share-menu-fix branch from 6ea81a1 to 2500c2f Compare April 25, 2018 04:45

NejcZdovc assigned ryanml Apr 25, 2018

bsclifton self-requested a review May 1, 2018 05:27

bsclifton added this to the 0.22.x Release 3 (Beta channel) milestone May 1, 2018

LaurenWags mentioned this pull request May 1, 2018

error in terminal when selecting File > Share without any open windows #13928

Closed

bsclifton reviewed May 1, 2018

View reviewed changes

bsclifton suggested changes May 1, 2018

View reviewed changes

petemill suggested changes May 1, 2018

View reviewed changes

ryanml force-pushed the file-share-menu-fix branch 2 times, most recently from eacab52 to 99cba57 Compare May 1, 2018 19:10

bsclifton previously approved these changes May 1, 2018

View reviewed changes

petemill previously approved these changes May 1, 2018

View reviewed changes

ryanml dismissed stale reviews from petemill and bsclifton via 191cdb0 May 1, 2018 22:08

ryanml added 2 commits May 1, 2018 15:10

Ensuring share context menu items are disabled when no windows are open

86d1a11

Refactoring menu.js item attribute functions

Removing digg from templateUrls

f371e51

ryanml force-pushed the file-share-menu-fix branch from 1c40501 to f371e51 Compare May 1, 2018 22:11

petemill approved these changes May 1, 2018

View reviewed changes

bsclifton merged commit ca83ee3 into brave:master May 1, 2018

bsclifton added a commit that referenced this pull request May 1, 2018

Merge pull request #13933 from ryanml/file-share-menu-fix

1954b5c

Ensuring File > Share context menu items are disabled when no windows are open

bsclifton added a commit that referenced this pull request May 1, 2018

Merge pull request #13933 from ryanml/file-share-menu-fix

4403af2

Ensuring File > Share context menu items are disabled when no windows are open

bsclifton mentioned this pull request May 2, 2018

Cannot set property 'enabled' of null when launching or closing Brave. #13991

Closed

bsclifton mentioned this pull request Jul 2, 2018

Refactoring menu #13187

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensuring File > Share context menu items are disabled when no windows are open #13933

Ensuring File > Share context menu items are disabled when no windows are open #13933

ryanml commented Apr 25, 2018 •

edited

Loading

codecov-io commented Apr 25, 2018 •

edited

Loading

bsclifton May 1, 2018

bsclifton left a comment

bsclifton May 1, 2018

petemill left a comment

petemill May 1, 2018

bsclifton May 1, 2018

ryanml commented May 1, 2018

bsclifton left a comment

petemill left a comment

petemill May 1, 2018

ryanml May 1, 2018

petemill left a comment

bsclifton commented May 1, 2018

Ensuring File > Share context menu items are disabled when no windows are open #13933

Ensuring File > Share context menu items are disabled when no windows are open #13933

Conversation

ryanml commented Apr 25, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

codecov-io commented Apr 25, 2018 • edited Loading

Codecov Report

bsclifton May 1, 2018

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton May 1, 2018

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

petemill May 1, 2018

Choose a reason for hiding this comment

bsclifton May 1, 2018

Choose a reason for hiding this comment

ryanml commented May 1, 2018

bsclifton left a comment

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

petemill May 1, 2018

Choose a reason for hiding this comment

ryanml May 1, 2018

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

bsclifton commented May 1, 2018

ryanml commented Apr 25, 2018 •

edited

Loading

codecov-io commented Apr 25, 2018 •

edited

Loading