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)
Hello (Loop)
Client
Tracking
(firefox34 verified, firefox35 verified)
VERIFIED
FIXED
mozilla35
People
(Reporter: NiKo, Unassigned)
References
Details
(Whiteboard: [loop-uplift][fig:wontverify])
Attachments
(1 file, 7 obsolete files)
38.92 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
|
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 | ||
Updated•10 years ago
|
Assignee: nobody → nperriault
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8483354 -
Flags: feedback?(dmose)
Comment 2•10 years ago
|
||
Andrei may have some thoughts here; adding him to the CC.
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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?
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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-
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
Updated patch with the right changeset. Sorry :/
Attachment #8487921 -
Attachment is obsolete: true
Attachment #8487921 -
Flags: review?(mdeboer)
Attachment #8487983 -
Flags: review?(mdeboer)
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
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
Comment 19•10 years ago
|
||
Target Milestone: --- → mozilla35
Comment 20•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 21•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
(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)
Updated•10 years ago
|
Whiteboard: [qablocked: bug 979845] → [qablocked: bug 979845][loop-uplift]
Comment 24•10 years ago
|
||
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)
Comment 25•10 years ago
|
||
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)
Comment 26•10 years ago
|
||
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)
Comment 27•10 years ago
|
||
(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)
Comment 28•10 years ago
|
||
Ok, enough of the dependencies of bug 979845 have been fixed to verify what was mentioned in comment 22.
Comment 29•10 years ago
|
||
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
status-firefox35:
--- → verified
Whiteboard: [qablocked: bug 979845][loop-uplift] → [loop-uplift]
Comment 30•10 years ago
|
||
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 31•10 years ago
|
||
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 32•10 years ago
|
||
Updated•10 years ago
|
status-firefox34:
--- → fixed
Comment 33•10 years ago
|
||
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+
Comment 34•10 years ago
|
||
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.
Description
•