Closed Bug 1214582 Opened 4 years ago Closed 4 years ago

Adjust how room titles are displayed/managed

Categories

(Hello (Loop) :: Client, defect, P2)

defect

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.3 - Nov 2
Tracking Status
firefox44 --- fixed

People

(Reporter: standard8, Assigned: mancas)

References

Details

(Whiteboard: [web sharing])

User Story

Acceptance Criteria:

- The display of room title should be as follows in order of priority:
-- Room Name
-- Context title if saved.
-- Fallback TBD
- The room title is display according to the priority in the panel list, the conversation window title and the standalone window.
- The standalone window format should remain the same with the Hello name in the title as well.

Attachments

(1 file, 2 obsolete files)

As part of the user journey rework, we need to adjust how the room titles are displayed/managed. This depends on bug 1212357 to avoid some bitrot.

See the user story for more detail.
Sevaan, what should we use as a fallback string if the room hasn't been given a name, and the shared web page doesn't have a title?
Flags: needinfo?(sfranks)
Blocks: 1214590
Note: bug 1214590 will stop assigning the room names to rooms. For this we'll need to cover the non-room-name case via testing, which we'll need to do anyway.
Rank: 15
(In reply to Mark Banner (:standard8) from comment #1)
> Sevaan, what should we use as a fallback string if the room hasn't been
> given a name, and the shared web page doesn't have a title?

Let's use the URL in that case. It will look a little ugly, but at least it will have some meaning.
Flags: needinfo?(sfranks)
Assignee: nobody → b.mcb
Status: NEW → ASSIGNED
Hey Mark! This patch is ready to review
Attachment #8674828 - Flags: review?(standard8)
Comment on attachment 8674828 [details] [diff] [review]
Adjust how room titles are displayed/managed

Review of attachment 8674828 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, but we also need to handle the conversation window title (DesktopRoomConversationView) and the standalone one (StandaloneRoomView)
Attachment #8674828 - Flags: review?(standard8) → feedback+
When the work is complete please confirm that the context menu problem from Bug 1213846 isn't reproduced in the new design (the bug demonstrated in this screenshot: https://bug1213846.bmoattachments.org/attachment.cgi?id=8672606)
(In reply to Ian Bicking (:ianb) from comment #6)
> When the work is complete please confirm that the context menu problem from
> Bug 1213846 isn't reproduced in the new design (the bug demonstrated in this
> screenshot: https://bug1213846.bmoattachments.org/attachment.cgi?id=8672606)

That bug is all about positioning of the drop-down. It isn't going to be affected by room titles - and not related to the code in this bug. I think we should keep it as a separate issue unless you disagree Ian?
Flags: needinfo?(ianb)
Perhaps we could fix that problem  in bug 1212357 which is related to the dropdown.
Mark could you check if the patch works fine? If there is something wrong let me know.

Also I've added unit test for each part.

Thanks!
Attachment #8674828 - Attachment is obsolete: true
Attachment #8677276 - Flags: review?(standard8)
Comment on attachment 8677276 [details] [diff] [review]
Adjust how room titles are displayed/managed

Review of attachment 8677276 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable, but I think there's some tidy up that can be done. Unfortunately there's some bit rot due to the eslint patches that recently landed, although addressing the comments in the test will probably simplify a lot of it.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +743,5 @@
>        });
>      });
> +
> +    describe("Room name priority", function() {
> +      it("should use room name", function() {

I think it would be worth changing this test to "should use the room name by default", and extend the decryptedContext here to set a context url.

@@ +744,5 @@
>      });
> +
> +    describe("Room name priority", function() {
> +      it("should use room name", function() {
> +        var roomEntry = mountRoomEntry({

Please move the mountRoomEntry to a beforeEach - its the same in all three of these `it` sections.

@@ +748,5 @@
> +        var roomEntry = mountRoomEntry({
> +          dispatcher: dispatcher,
> +          room: new loop.store.Room(roomData)
> +        });
> +        var updatedRoom = new loop.store.Room(_.extend({}, roomData, {

It might be nice to create a utility function for this in-line (similar to mountRoomEntry), that just gets passed the decryptedContext data.

@@ +759,5 @@
> +        roomEntry.setProps({room: updatedRoom});
> +
> +        expect(
> +          roomEntry.getDOMNode().textContent)
> +        .eql("Room name");

I realise you copied & pasted this formatting, but I think this is short enough that it can all go on one line (and the instances in the other two tests).

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +653,5 @@
> +        it("should use room name", function() {
> +          activeRoomStore.setStoreState({
> +            participants: [{}],
> +            roomState: ROOM_STATES.JOINED,
> +            roomName: "fakeName"

I think it would be worth changing this test to "should use the room name by default", and extend the store state here to set a context url.

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +140,5 @@
>  
>      it("should display a join room button if the state is not ROOM_JOINED", function() {
>        activeRoomStore.setStoreState({
> +        roomState: ROOM_STATES.READY,
> +        roomName: "fakeName"

I think we should set the room name in a beforeEach section, seeing as you're setting it for virtually every test in this section.

If a test explicitly doesn't want it, then you can do setStoreState({ roomName: null }) or something like that.

@@ +429,5 @@
>      describe("#componentWillUpdate", function() {
>        it("should set document.title to roomName and brand name when the READY state is dispatched", function() {
>          activeRoomStore.setStoreState({roomName: "fakeName", roomState: ROOM_STATES.INIT});
>          view = mountTestComponent();
> +        activeRoomStore.setStoreState({roomState: ROOM_STATES.READY, roomName: "fakeName"});

Again, I think we can set the roomName in a beforeEach. Additionally, you shouldn't have to set it twice in an it() function - once should be enough.

@@ +526,5 @@
>          it("should send the second `TileShown` after ending and rejoining",
>            function() {
>              // Trigger the first message then rejoin and wait
>              clock.tick(1);
> +            activeRoomStore.setStoreState({roomState: ROOM_STATES.ENDED, roomName: "fakeName"});

Ditto with the beforeEach.

@@ +600,5 @@
>  
>        describe("Empty room message", function() {
>          it("should not display an message immediately in the JOINED state",
>            function() {
> +            activeRoomStore.setStoreState({roomState: ROOM_STATES.JOINED, roomName: "fakeName"});

You're already setting the room name in the before each here, so you shouldn't have to repeat it.

Please also check the rest of this file for unnecessary repeats.
Attachment #8677276 - Flags: review?(standard8) → review-
Hey Mark, I've addressed all of your comments. Please review that patch when you get a chance.

Thanks!
Attachment #8677276 - Attachment is obsolete: true
Attachment #8678764 - Flags: review?(standard8)
Comment on attachment 8678764 [details] [diff] [review]
Adjust how room titles are displayed/managed

Review of attachment 8678764 [details] [diff] [review]:
-----------------------------------------------------------------

That's better, r=Standard8
Attachment #8678764 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/839ec883c9dc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1213851
Get error now on localhost:
...
Loop:promiseRegisteredWithServers: registration already completed or in progress: 2 MozLoopService.jsm:449
Loop:hawkRequestInternal:  2 /rooms GET MozLoopService.jsm:608
Loop:notifyStatusChanged with reason: room-add MozLoopService.jsm:308
[Dispatcher] Dispatching action Object { roomList: Array[143], name: "updateRoomList" } dispatcher.js:92:9
[Dispatcher] Dispatching action caused an exception:  TypeError: this.props.room.decryptedContext.urls is undefined
Stack trace:
RoomEntry<.render@chrome://browser/content/loop/js/panel.js:430:1
[40]</L._renderValidatedComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:19780
[40]</L.mountComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:16175
[69]</v.Mixin.mountChildren@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:26548
[45]</i.Mixin._createContentMarkup@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:26397
[45]</i.Mixin.mountComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:25684
[69]</v.Mixin._mountChildByNameAtIndex@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:27885
[69]</v.Mixin._updateChildren@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:27290
[69]</v.Mixin.updateChildren@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:26878
[45]</i.Mixin._updateDOMChildren@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:28149
[45]</i.Mixin.updateComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:26750
[37]</d.Mixin.performUpdateIfNecessary@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:10127
[37]</d.Mixin.receiveComponent@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:9886
[45]</i.Mixin.receiveComponent@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:26525
[40]</L.updateComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:19107
[40]</L._performComponentUpdate@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:18643
[40]</L.performUpdateIfNecessary@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:18258
a@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:11401
[104]</r.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:27988
[104]</r.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:27988
[88]</<.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:12567
[88]</M<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:12797
[104]</r.closeAll@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:28609
[104]</r.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:28068
[54]</p.batchedUpdates@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:7657
s@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:11690
[40]</L.replaceState@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:16904
[40]</L.setState@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:16743
RoomList<._onStoreStateChanged@chrome://browser/content/loop/js/panel.js:666:7
triggerEvents@chrome://browser/content/loop/shared/libs/backbone-1.2.1.js:351:32
triggerApi@chrome://browser/content/loop/shared/libs/backbone-1.2.1.js:339:19
eventsApi@chrome://browser/content/loop/shared/libs/backbone-1.2.1.js:137:1
Events.trigger@chrome://browser/content/loop/shared/libs/backbone-1.2.1.js:329:1
baseStorePrototype.setStoreState@chrome://browser/content/loop/shared/js/store.js:54:7
loop.store.RoomStore<.updateRoomList@chrome://browser/content/loop/js/roomStore.js:487:1
Dispatcher.prototype._dispatchNextAction/<@chrome://browser/content/loop/shared/js/dispatcher.js:97:11
Dispatcher.prototype._dispatchNextAction@chrome://browser/content/loop/shared/js/dispatcher.js:95:7
Dispatcher.prototype.dispatch@chrome://browser/content/loop/shared/js/dispatcher.js:69:7
baseStorePrototype.dispatchAction@chrome://browser/content/loop/shared/js/store.js:29:7
loop.store.RoomStore<.getAllRooms/<@chrome://browser/content/loop/js/roomStore.js:464:9
Fixed this by navigating to loop-server/tools editing and running the following command:

edit remove_old_keys.js

Change var to following:
var TWO_WEEKS = 1;

Save then run:
node remove_old_keys.js

This will remove all keys and you will need to sign-up for account again.
You can revert remove_old_keys.js if needed after running this.

Mark is filling a bug on fall back or default for about:_blank context condition and no room name.
Flags: needinfo?(standard8)
Flags: needinfo?(b.mcb)
From my point of view, we should not allow the users to create rooms without context. I mean, about:blank, etc. pages. Since our main goal is to share the web, it has no sense to share a tab with no content.

What do you think about it?
Flags: needinfo?(b.mcb)
Blocks: 1219600
(In reply to Manuel Casas Barrado [:mancas] from comment #17)
> From my point of view, we should not allow the users to create rooms without
> context. I mean, about:blank, etc. pages. Since our main goal is to share
> the web, it has no sense to share a tab with no content.

Whilst I see your point, I think that not allowing them to create rooms in particular states is potentially going to cause confusion on the part of the user.

I've filed bug 1219600 for follow-up about this, as I think we need to handle fallback not just in the about cases but also for general issues such as not being able to decrypt our own data (for whatever reason) or server loss issues.
Flags: needinfo?(standard8)
Flags: needinfo?(ianb)
Iteration: --- → 44.3 - Nov 2
You need to log in before you can comment on or make changes to this bug.