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

Use muon's tab_strip_model for driving index and active #10640

Merged
merged 2 commits into from
Aug 31, 2017
Merged

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Aug 23, 2017

Sister PR that is rebased on 0.18.x milestone.
This fix first landed in 0.18.x hotfix.
It is only tracked in 0.19.x because it's rebased differently and we want to not lose track of the fixes for uplifting once 4.4 muon builds are ready.

Fix #9083
Fix #9385
Fix #9671
Fix #9722
Fix #10038
Fix #10384
Fix #10436
Fix #10532
Fix #10561

and probably several others.

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

@bbondy bbondy added this to the 0.18.x Hotfix (Release Channel) milestone Aug 23, 2017
@bbondy bbondy self-assigned this Aug 23, 2017
@bbondy bbondy requested a review from bridiver August 23, 2017 13:47
@codecov-io
Copy link

codecov-io commented Aug 23, 2017

Codecov Report

Merging #10640 into master will decrease coverage by 0.34%.
The diff coverage is 37.95%.

@@            Coverage Diff             @@
##           master   #10640      +/-   ##
==========================================
- Coverage   54.41%   54.07%   -0.35%     
==========================================
  Files         246      247       +1     
  Lines       21368    21522     +154     
  Branches     3320     3332      +12     
==========================================
+ Hits        11628    11638      +10     
- Misses       9740     9884     +144
Flag Coverage Δ
#unittest 54.07% <37.95%> (-0.35%) ⬇️
Impacted Files Coverage Δ
app/renderer/lib/tabUtil.js 100% <ø> (ø) ⬆️
js/actions/windowActions.js 5.76% <ø> (+0.05%) ⬆️
js/constants/appConstants.js 100% <ø> (ø) ⬆️
app/renderer/reducers/frameReducer.js 27.07% <0%> (+0.75%) ⬆️
js/contextMenus.js 17.92% <0%> (ø) ⬆️
js/actions/appActions.js 13.28% <0%> (-1.11%) ⬇️
js/stores/windowStore.js 28.59% <0%> (-1.7%) ⬇️
app/common/state/tabState.js 79.31% <100%> (+0.73%) ⬆️
app/renderer/components/tabs/tab.js 70.49% <20%> (ø) ⬆️
app/browser/windows.js 24.31% <33.33%> (-5.56%) ⬇️
... and 15 more

@@ -319,7 +318,7 @@ const frameOptsFromFrame = (frame) => {
* Adds a frame specified by frameOpts and newKey and sets the activeFrameKey
* @return Immutable top level application state ready to merge back in
*/
function addFrame (state, frameOpts, newKey, partitionNumber, openInForeground, insertionIndex) {
function addFrame (state, frameOpts, newKey, partitionNumber, openInForeground, insertionIndex, active) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

active doesn't appear to be used?

@bbondy bbondy force-pushed the tab-strip-master branch 2 times, most recently from 5805ca0 to 836d69e Compare August 29, 2017 12:56
@bbondy bbondy force-pushed the tab-strip-master branch 2 times, most recently from c008832 to 45da478 Compare August 30, 2017 20:03
@@ -158,6 +119,7 @@ const tabsReducer = (state, action, immutableAction) => {
break
case appConstants.APP_TAB_UPDATED:
state = tabState.maybeCreateTab(state, action)
// tabs.debugTabs(state)
Copy link
Member

@bsclifton bsclifton Aug 30, 2017

Choose a reason for hiding this comment

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

comment left in here 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

that's on purpose because I toggle it often

const windowIdOfTabBeingRemoved = tabState.getWindowId(state, tabId)
state = tabs.updateTabsStateForWindow(state, windowIdOfTabBeingRemoved)
}
console.log('calling removeTabByTabId ofr tabId:', tabId)
Copy link
Member

Choose a reason for hiding this comment

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

console.log that we can remove

break
}
case appConstants.APP_TAB_MOVED:
state = tabs.updateTabsStateForAttachedTab(state, action.get('tabId'))
Copy link
Member

Choose a reason for hiding this comment

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

appConstants.APP_TAB_MOVED can be grouped with appConstants.APP_TAB_ATTACHED (since they have the same code)

Copy link
Member Author

Choose a reason for hiding this comment

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

they are 2 diff things though so I don't think they should be.

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.

Haven't had a chance to try it (since we'd need a new Muon)- but code changes look good 😄 👍

Fix #10436
Fix #9385
Fix #9722
Fix #10561
Fix #9083
Fix #9671
Fix #10038
Fix #10384
Fix #10532

and probably several others.
@bbondy bbondy merged commit 8b6d64c into master Aug 31, 2017
bbondy added a commit that referenced this pull request Aug 31, 2017
Use muon's tab_strip_model for driving index and active
bbondy added a commit that referenced this pull request Aug 31, 2017
From a review comment on #10640
bbondy added a commit that referenced this pull request Aug 31, 2017
From a review comment on #10640
bbondy added a commit that referenced this pull request Aug 31, 2017
From a review comment on #10640
bbondy added a commit that referenced this pull request Aug 31, 2017
From a review comment on #10640
@bbondy bbondy deleted the tab-strip-master branch October 23, 2017 14:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.