Closed
Bug 1093620
Opened 8 years ago
Closed 8 years ago
Investigate using a single store for Loop Rooms
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(firefox35 fixed, firefox36 fixed)
People
(Reporter: NiKo, Unassigned)
Details
Attachments
(1 file, 2 obsolete files)
69.29 KB,
patch
|
standard8
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Separating stores for rooms makes sharing code logic harder than necessary; we should investigate using the Cursor pattern (a cursor is basically a piece of the application state that knows how to update itself) to allow using a single store (and substores) to ease subscribing to substate changes, while substores could implement their very own logic.
Assignee | ||
Comment 1•8 years ago
|
||
Basic proof of concept of the idea.
Attachment #8516686 -
Flags: feedback?(standard8)
Comment 2•8 years ago
|
||
Comment on attachment 8516686 [details] [diff] [review] Using a substore for Rooms (PoC). Review of attachment 8516686 [details] [diff] [review]: ----------------------------------------------------------------- Generally looking good, I think its worth moving forward into code, as it seems a better way of doing things and we'll only know for sure once we start using it. ::: browser/components/loop/content/js/roomViews.jsx @@ +25,4 @@ > }, > > componentWillMount: function() { > + this.listenTo(this.props.roomStore, "change:localStore", This needs to be consistent with the key name for the event in roomStore. @@ +35,5 @@ > * > * @private > */ > + _onActiveRoomStoreChanged: function() { > + this.setState(this.props.roomStore.getStoreState().localStore); localStore -> proper name. ::: browser/components/loop/content/shared/js/roomStore.js @@ +68,5 @@ > } > this._mozLoop = options.mozLoop; > > + if (!options.activeRoomStore) { > + options.activeRoomStore = new loop.store.ActiveRoomStore({ As discussed, this should be an optional parameter. @@ +114,5 @@ > error: null, > pendingCreation: false, > pendingInitialRetrieval: false, > + rooms: [], > + localRoom: {} Need to change localRoom -> activeRoom throughout the file. @@ +163,5 @@ > this._mozLoop.rooms.on("delete", this._onRoomRemoved.bind(this)); > }, > > /** > + * Updates local room store state. local -> active
Attachment #8516686 -
Flags: feedback?(standard8) → feedback+
Assignee | ||
Comment 3•8 years ago
|
||
Addressed comments, added more tests. This is now ready for review I think.
Attachment #8516686 -
Attachment is obsolete: true
Attachment #8516805 -
Flags: review?(standard8)
Assignee | ||
Comment 4•8 years ago
|
||
Updated patch to remove all references to local room ids (now obsolete). Added more tests.
Attachment #8516805 -
Attachment is obsolete: true
Attachment #8516805 -
Flags: review?(standard8)
Attachment #8516958 -
Flags: review?(standard8)
Comment 5•8 years ago
|
||
Comment on attachment 8516958 [details] [diff] [review] Using a single root store for Rooms. Review of attachment 8516958 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=Standard8
Attachment #8516958 -
Flags: review?(standard8) → review+
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9a1524fcdbb0
Assignee: nobody → nperriault
Iteration: --- → 36.2
Target Milestone: --- → mozilla36
Comment 7•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9a1524fcdbb0
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
status-firefox35:
--- → fixed
Comment 9•8 years ago
|
||
Comment on attachment 8516958 [details] [diff] [review] Using a single root store for Rooms. Approval Request Comment Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8516958 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Attachment #8516958 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•