Closed Bug 1062126 Opened 10 years ago Closed 10 years ago

Loop panel UI shouldn't be fully reset when reopened

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal

Tracking

(firefox34 verified, firefox35 verified)

VERIFIED FIXED
mozilla35
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: NiKo, Unassigned)

References

Details

(Whiteboard: [loop-uplift][fig:wontverify])

Attachments

(1 file, 7 obsolete files)

38.92 KB, patch
Details | Diff | Splinter Review
After bug 994146 landed, we've seen the whole panel UI being fully reset when reopened; this is probably overkill as what we really want is simply to fetch a new call URL. Also, it gets in the way of handling state in the UI, especially when it comes to user authentication status. So we need to keep listening to the visibility change event, but just trigger a new call URL fetching from there and propagate the result to the call URL input text.
Assignee: nobody → nperriault
This WiP patch brings an AppEventDispatcher to the Panel, with an associated react mixin; this is a first step towards extracting business logic from the UI one, easing dealing with dependencies and propagating app events to deeply nested subcomponents. This also fixes the issue for current bug. I haven't updated tests yet because I'd love to get feedback first :)
Attachment #8483354 - Flags: feedback?(standard8)
Attachment #8483354 - Flags: feedback?(rgauthier)
Andrei may have some thoughts here; adding him to the CC.
Comment on attachment 8483354 [details] [diff] [review] Prevent resetting the whole panel UI when reopened (WiP). Review of attachment 8483354 [details] [diff] [review]: ----------------------------------------------------------------- As it sits, I find this patch adds enough complexity to the code that it makes reasoning about this stuff more difficult. That said, there a couple of things that are unclear to me, and it's possible that tweaking them would solve this problem. Shouldn't the mixin be called AppEventListenerMixin, given that it offers an object the ability to listen for an AppEvent, and doesn't really do any dispatching at all? As far as I can tell, there is only ever one dispatcher. Why parameterize the dispatcher as a prop, rather than just baking specific knowledge of it into the mixin? This seems like it could make the code notably simpler to reason about. I'd be interested in hearing your thoughts and seeing a patch with either or both of the above changes... ::: browser/components/loop/content/js/panel.jsx @@ -287,5 @@ > - // If we've already got a callURL, don't bother requesting a new one. > - // As of this writing, only used for visual testing in the UI showcase. > - if (this.state.callUrl.length) { > - return; > - } Has the patch removed the above semantics intentionally, or am I misunderstanding? @@ +405,5 @@ > /** > * FxA sign in/up link component. > */ > var AuthLink = React.createClass({ > + mixins: [AppEventDispatcherMixin], Why would AuthLink want this mixin?
Attachment #8483354 - Flags: feedback?(dmose) → feedback-
Comment on attachment 8483354 [details] [diff] [review] Prevent resetting the whole panel UI when reopened (WiP). Review of attachment 8483354 [details] [diff] [review]: ----------------------------------------------------------------- I'm realizing that it's not obvious to me that we'd have other users of the mixin any time soon, but maybe I just don't have the right use cases in my head. If, however, that's true, is it premature to extract this code into a mixin, since it decouples things from the only caller? ::: browser/components/loop/content/js/panel.jsx @@ +69,4 @@ > * Availability drop down menu subview. > */ > var AvailabilityDropdown = React.createClass({ > + mixins: [DropdownMenuMixin, AppEventDispatcherMixin], Why would this want the mixin? @@ +200,5 @@ > /** > * Panel settings (gear) menu. > */ > var SettingsDropdown = React.createClass({ > + mixins: [DropdownMenuMixin, AppEventDispatcherMixin], Why would this want the mixin?
(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #4) > I'm realizing that it's not obvious to me that we'd have other users of the > mixin any time soon, but maybe I just don't have the right use cases in my > head. > > If, however, that's true, is it premature to extract this code into a mixin, > since it decouples things from the only caller? This was not a patch ready for landing, it was a minimal start towards extracting business logic and external dependencies from the UI components — hence why the event dispatcher doesn't actually dispatch anything, just listen for now. So as this is visibly too incomplete to be meaningful, I'll update the patch so the whole panel UI would benefit from it.
After discussing with the team, we decided than this is urgent enough that we need to find a simpler approach to fix the current issue. Bug 1064279 will take over investigating using a Flux-like approach along with react components in Loop.
No longer depends on: 1064279
Comment on attachment 8483354 [details] [diff] [review] Prevent resetting the whole panel UI when reopened (WiP). I think any feedback I would give here now would be obsolete.
Attachment #8483354 - Flags: feedback?(standard8)
This patch adds a new DocumentVisibilityMixin which wraps the document visibility event listening processus, so we can share & reuse it. While tests have been written, they're disabled because once can't simply dispatch a fake document event (the event `target`property is read only). We should file a new bug to find a proper solution to this problem, but I'd love hearing thoughts from other people before.
Attachment #8486425 - Flags: review?(standard8)
This is kind of a possible part 2 for this patch, where I introduce a way to fake the root object used by the mixins brought by the first patch, so we can actually enabled the tests I wrote and check that the applied mixins work as expected. I'm definitely not sure this is the best way, but: - it tests things - it works I'm wondering how hazardous is it to introduce that setRootObject method in the mixins module but can't see much problem; anyway, any thought is welcome here.
Attachment #8486747 - Flags: feedback?(standard8)
Attachment #8486747 - Flags: feedback?(mdeboer)
Comment on attachment 8486747 [details] [diff] [review] attempt at faking native DOM events. Review of attachment 8486747 [details] [diff] [review]: ----------------------------------------------------------------- I like the approach, Niko, I don't really see a problem with this right away. ::: browser/components/loop/content/shared/js/mixins.js @@ +21,5 @@ > + * > + * @param {Object} > + */ > + function setRootObject(obj) { > + console.info("loop.shared.mixins: rootObject set to " + obj); you can change this to `"loop.shar...", obj);` to be able to inspect the obj some more in the console.
Attachment #8486747 - Flags: feedback?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #10) > I like the approach, Niko, I don't really see a problem with this right away. Right, the more I stare at this patch, the more I'm convinced there's no other simple way to do that. We can always revisit later anyway :) I'll attach a new patch merging the two previous ones. > you can change this to `"loop.shar...", obj);` to be able to inspect the obj > some more in the console. Good point, will fix.
New patch rebased on latest fx-team and adding the missing tests.
Attachment #8483354 - Attachment is obsolete: true
Attachment #8486425 - Attachment is obsolete: true
Attachment #8486747 - Attachment is obsolete: true
Attachment #8483354 - Flags: feedback?(rgauthier)
Attachment #8486425 - Flags: review?(standard8)
Attachment #8486747 - Flags: feedback?(standard8)
Attachment #8487410 - Flags: review?(mdeboer)
Comment on attachment 8487410 [details] [diff] [review] Loop panel UI shouldn't be fully reset when reopened. Review of attachment 8487410 [details] [diff] [review]: ----------------------------------------------------------------- r=me when the bug I mentioned to you about the jumping panel content is fixed in the next patch. I still would like to check that. ::: browser/components/loop/content/js/panel.jsx @@ +12,5 @@ > "use strict"; > > var sharedViews = loop.shared.views, > sharedModels = loop.shared.models, > + sharedMixins = loop.shared.mixins, I'll allow it here, but it's bad practice to share declarations in one var statement. You'll get additional brownie points when you land this patch with this unraveled to multiple var statements. ::: browser/components/loop/content/shared/js/views.jsx @@ +11,5 @@ > loop.shared.views = (function(_, OT, l10n) { > "use strict"; > > + var sharedModels = loop.shared.models, > + sharedMixins = loop.shared.mixins; nit: one var declaration per line, please.
Attachment #8487410 - Flags: review?(mdeboer) → review-
Updated patch to prevent a strange race condition in the NotificationListView component when two different update events are triggered from the collection in a within a very thin time window.
Attachment #8487410 - Attachment is obsolete: true
Attachment #8487921 - Flags: review?(mdeboer)
Updated patch with the right changeset. Sorry :/
Attachment #8487921 - Attachment is obsolete: true
Attachment #8487921 - Flags: review?(mdeboer)
Attachment #8487983 - Flags: review?(mdeboer)
Comment on attachment 8487983 [details] [diff] [review] Loop panel UI shouldn't be fully reset when reopened. Review of attachment 8487983 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, ship it! ::: browser/components/loop/content/shared/js/views.jsx @@ +11,5 @@ > loop.shared.views = (function(_, OT, l10n) { > "use strict"; > > + var sharedModels = loop.shared.models, > + sharedMixins = loop.shared.mixins; please land this with a var declaration for each constant
Attachment #8487983 - Flags: review?(mdeboer) → review+
Latest nits addressed, patch is ready to land. :Standard8, could you please take care of landing this for me, because it's urgent? Thanks.
Attachment #8487983 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
What is the test to verify this is working as expected? Is it simply to quickly open and close the "Invite someone to talk" panel to make sure the URL is refetched each time?
Flags: qe-verify+
Flags: needinfo?(nperriault)
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #21) > What is the test to verify this is working as expected? Is it simply to > quickly open and close the "Invite someone to talk" panel to make sure the > URL is refetched each time? Yes, basically. Once we land & enable FxA integration (bug 979845), we'll to ensure that when the panel is reopened the authentication status is kept in the footer, and that only the call URL field is updated.
Flags: needinfo?(nperriault)
Blocking verification on bug 979845.
Whiteboard: [qablocked: bug 979845]
Whiteboard: [qablocked: bug 979845] → [qablocked: bug 979845][loop-uplift]
QE is still blocked by bug 979845 to verify this is fixed. This is concerning considering we want this uplifted to Aurora in the next couple days.
Flags: needinfo?(mreavy)
Matt -- In order to unblock testing of bugs like this, does it make sense to make bug 979845 about the FxA work we need for Fx34 and create a follow on meta for the remaining FxA work that we'd like to fix soon, but isn't critical for a first release? I'm thinking it makes a lot of sense, but I wanted your thoughts. If you agree, can you update bug 979845 to reflect that?
Flags: needinfo?(mreavy) → needinfo?(MattN+bmo)
Bug 979845 is a meta bug and I don't see how it's blocking verification of this. Can you explain?
Flags: needinfo?(MattN+bmo) → needinfo?(anthony.s.hughes)
(In reply to Matthew N. [:MattN] from comment #26) > Bug 979845 is a meta bug and I don't see how it's blocking verification of > this. Can you explain? See comment 22.
Flags: needinfo?(anthony.s.hughes)
Ok, enough of the dependencies of bug 979845 have been fixed to verify what was mentioned in comment 22.
Thanks Matt. I've unblocked this and quickly checked that my user name remains displayed in Nightly when authenticated and reopening the panel several times. Marking this verified fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [qablocked: bug 979845][loop-uplift] → [loop-uplift]
This is not high priority for verification before uplift so I'm tagging it as such. We'll retest once it's uplifted to Aurora.
Whiteboard: [loop-uplift] → [loop-uplift][fig:wontverify]
Comment on attachment 8488144 [details] [diff] [review] Loop panel UI shouldn't be fully reset when reopened, r=mikedeboer. Approval Request Comment Uplift request for patches staged and tested on Fig
Attachment #8488144 - Flags: approval-mozilla-aurora?
Comment on attachment 8488144 [details] [diff] [review] Loop panel UI shouldn't be fully reset when reopened, r=mikedeboer. I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.
Attachment #8488144 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I've noticed this to be working in Aurora over the last few days.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: