Closed Bug 1239972 Opened 9 years ago Closed 8 years ago

Adapt infobar message based on whether a link clicker is in the room or not

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Iteration:
48.3 - Apr 25

People

(Reporter: RT, Assigned: crafuse)

References

Details

User Story

We realized that it can be confusing that the infobar states "You are sharing your tabs. Any tab you click on can be seen by your friends" regardless of whether someone else is in the room or not.

We need 2 different copies:
- Copy 1: the link generator is alone in the room, he needs to be informed that when someone else will join him, that person will see his tabs
Copy for implementation: "As soon as your friend joins, they will be able to see any tab you click on."

- Copy 2: the link generator and the link clicker are together in the room, the link generator needs to be informed that his tabs can be seen by the link clicker.
Copy for implementation: "You are now sharing your tabs. Your friend will see any tab you click on."

Attachments

(6 files, 2 obsolete files)

      No description provided.
User Story: (updated)
Matej could you please help us here with appropriate copies for the scenarios mentionned above?
Rank: 25
Flags: needinfo?(matej)
Priority: -- → P2
(In reply to Romain Testard [:RT] from comment #1)
> Matej could you please help us here with appropriate copies for the
> scenarios mentionned above?

How about the following?

> - Copy 1: the link generator is alone in the room, he needs to be informed that when someone else will
> join him, that person will see his tabs

"As soon as your friend joins, they will be able to see any tab you click on."


> - Copy 2: the link generator and the link clicker are together in the room, the link generator needs to 
> be informed that his tabs can be seen by the link clicker.

"You are now sharing your tabs. Your friend will see any tab you click on."
Flags: needinfo?(matej)
Whiteboard: [triage]
Assignee: nobody → crafuse
(In reply to Matej Novak [:matej] from comment #2)
> (In reply to Romain Testard [:RT] from comment #1)
> > Matej could you please help us here with appropriate copies for the
> > scenarios mentionned above?
> 
> How about the following?
> 
> > - Copy 1: the link generator is alone in the room, he needs to be informed that when someone else will
> > join him, that person will see his tabs
> 
> "As soon as your friend joins, they will be able to see any tab you click
> on."
> 
> 
> > - Copy 2: the link generator and the link clicker are together in the room, the link generator needs to 
> > be informed that his tabs can be seen by the link clicker.
> 
> "You are now sharing your tabs. Your friend will see any tab you click on."

Looks good to me, just made a change to "As soon as your friend joins, he will be able to see any tab you click on." (replaced "they") and updated the user story field. If this is good with you then it's ready for implementation.
User Story: (updated)
(In reply to Romain Testard [:RT PTO until Feb 1st] from comment #3)
> (In reply to Matej Novak [:matej] from comment #2)
> > (In reply to Romain Testard [:RT] from comment #1)
> > > Matej could you please help us here with appropriate copies for the
> > > scenarios mentionned above?
> > 
> > How about the following?
> > 
> > > - Copy 1: the link generator is alone in the room, he needs to be informed that when someone else will
> > > join him, that person will see his tabs
> > 
> > "As soon as your friend joins, they will be able to see any tab you click
> > on."
> > 
> > 
> > > - Copy 2: the link generator and the link clicker are together in the room, the link generator needs to 
> > > be informed that his tabs can be seen by the link clicker.
> > 
> > "You are now sharing your tabs. Your friend will see any tab you click on."
> 
> Looks good to me, just made a change to "As soon as your friend joins, he
> will be able to see any tab you click on." (replaced "they") and updated the
> user story field. If this is good with you then it's ready for
> implementation.

I would recommend switching this back to "they" or potentially using "she."
Oops, now fixed, my mistake!
User Story: (updated)
Rank: 25 → 24
Whiteboard: [triage]
create bug to remove the following string after new strings are implemented:
infobar_screenshare_browser_message2
Comment on attachment 8717520 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-if-link-clicker-in-room-or-not > mozilla:master

Strings section.
Attachment #8717520 - Flags: review?(dmose)
Comment on attachment 8717520 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-if-link-clicker-in-room-or-not > mozilla:master

r=dmose
Attachment #8717520 - Flags: review?(dmose) → review+
Starting infobar guest in room status message change.
Status: NEW → ASSIGNED
Updating priority to reflect the fact we want this one done in our sprint commencing this week.
Rank: 24 → 9
Priority: P2 → P1
Rank: 9 → 11
Blocks: 1248602
Blocks: 1248604
No longer blocks: 1248602
Should this message be the same when I am paused and un-paused in an empty room?  Back ground colour is different when paused.
Attachment #8726865 - Flags: review?(rtestard)
Attached image Unpaused Empty Room
Comment on attachment 8726866 [details]
Unpaused Empty Room

Here is message for Un-paused in an empty room.
Attachment #8726866 - Attachment description: Unpaused Empty Room with si → Unpaused Empty Room
Attachment #8726866 - Flags: review?(rtestard)
Paused state and guest in room and not sharing tabs.
Attachment #8726868 - Flags: review?(rtestard)
(In reply to Chris Rafuse :crafuse from comment #12)
> Created attachment 8726865 [details]
> Paused Empty Room with similar message to Unpaused Empty Room
> 
> Should this message be the same when I am paused and un-paused in an empty
> room?  Back ground colour is different when paused.

This message should read differently if the user has paused sharing. Maybe:

"You have stopped sharing your tabs. When your friend joins, they won't be able to see anything until you restart sharing."



(In reply to Chris Rafuse :crafuse from comment #15)
> Created attachment 8726867 [details]
> Un-paused Guest In Room with join nofication shown in Ubuntu Linux - Live!
> 
> Maybe we need to communicate to users like in the broadcasting industry.

This is a fun idea, and I like it. Will explore.


(In reply to Chris Rafuse :crafuse from comment #16)
> Created attachment 8726868 [details]
> Paused Guest In Room and not sharing tabs.
> 
> Paused state and guest in room and not sharing tabs.

This should have a period at the end to be consistent with out other notification bars.
Comment on attachment 8726867 [details]
Un-paused Guest In Room with join nofication shown in Ubuntu Linux - Live!

Shouldn't the Firefox/Nightly logo be in that notification? Or it doesn't work like that in Linux?
Attachment #8726867 - Flags: review?(sfranks) → review+
Updated string.
Attachment #8728783 - Flags: ui-review?(sfranks)
Attachment #8726865 - Attachment is obsolete: true
Attachment #8726865 - Flags: review?(rtestard)
Attached image Paused Guest In Room
Added punctuation and will need translation.
Attachment #8726868 - Attachment is obsolete: true
Attachment #8726868 - Flags: review?(rtestard)
Attachment #8728792 - Flags: review?(sfranks)
Attachment #8728792 - Flags: review?(sfranks) → ui-review?(sfranks)
Attachment #8726866 - Flags: review?(rtestard) → ui-review?(sfranks)
Comment on attachment 8726901 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-based-on-link-clicker-in-the-room-or-not > mozilla:master

No tests yet but will create separate bug for test creation.
Attachment #8726901 - Flags: review?(dmose)
Attachment #8726867 - Flags: review?(rtestard)
Blocks: 1255525
Attachment #8726866 - Flags: ui-review?(sfranks) → ui-review+
Attachment #8728783 - Flags: ui-review?(sfranks) → ui-review+
Attachment #8728792 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8726901 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-based-on-link-clicker-in-the-room-or-not > mozilla:master

The notification bar is being re-shown (i.e. re-transitioned in) when it shouldn't be, which looks weird.  Chris and I have chatted about it, and he's pursuing the strategy described at https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Alerts_and_Notifications#Using_notification_box to fix.

Removing the r? for now.
Attachment #8726901 - Flags: review?(dmose)
Comment on attachment 8726901 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-based-on-link-clicker-in-the-room-or-not > mozilla:master

After discussion with Ian, we've decided that we can live with the transition regression.  Chris, can you please spin that out to another bug (unless it's already covered by the existing "flicker" bug, in which case that bug should have another acceptance criterion added to that user story so that it gets verified fixed.
Attachment #8726901 - Flags: review?(dmose)
Comment on attachment 8726901 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-based-on-link-clicker-in-the-room-or-not > mozilla:master

Feedback+ as a start.  Please create a branch with some of the suggestions we talked about and documented in the PR and re-request review so that I can get a sense of whether it feels more readable/maintainable, despite the necessary workaround.
Attachment #8726901 - Flags: review?(dmose) → feedback+
Rank: 11 → 10
Dan and I were reviewing https://github.com/mozilla/loop/pull/244
Came to the conclusions to:
A) fix looprooms structure
B) make it more unit testable
C) or just make it work with risk of coverage

Seeing in the time given I think the current code may work with one question:
Is this.switchTabNonOwnerParticipants an issue of being overwritten in this situation?
this.switchTabNonOwnerParticipants is to hold the state of guests in room while or after switching tabs 
or is there another way to store the state between tab switching operations?
Flags: needinfo?(standard8)
I discussed this with Chris and Dan yesterday, and we came up with a small fix to the LoopRooms structure that should allow a simpler fix to take place.
Flags: needinfo?(standard8)
Blocks: 1258334
No longer blocks: 1248604
Rank: 10 → 9
Comment on attachment 8726901 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-based-on-link-clicker-in-the-room-or-not > mozilla:master

Added Mochitests for infobar button type and messages in paused and un-paused states.
Attachment #8726901 - Flags: review?(dmose)
Comment on attachment 8726901 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-based-on-link-clicker-in-the-room-or-not > mozilla:master

A big step in the right direction; feedback+ and comments in the PR.
Attachment #8726901 - Flags: review?(dmose)
Any reason the mochitests are not outputting to console?  
Running this after git-export build of latest fx-team. 

./mach test browser/extensions/loop/chrome/test/mochitest/browser_mozLoop_sharingListeners.js | grep bar > ~/Documents/projects/infoBarTestOutput.txt

empty file.

The setup of a connection to the browser seems a large and will take a bit to create the connection.  Suggest we avert this test and create later as not to block existing progress.

Review comments have been addressed for infobar.
Flags: needinfo?(standard8)
Flags: needinfo?(dmose)
So, we figured out the blank file stuff yesterday, and now we're got a hypothesis of how we can get a test basically working and stood up, so removing the needinfos.
Flags: needinfo?(standard8)
Flags: needinfo?(dmose)
Added infobar tests to browser_LoopRooms_channel.js, fails on no room found after setting up a room and using ROOM_TOKEN to match the roomToken generated but fails.  It seems it is required to now have a room in the rooms array exist to test the infobar at all.  Original tests fail now.  Any suggestions on how to connect or fake a rooms list with the containing room?
Flags: needinfo?(standard8)
Commented in the PR.
Flags: needinfo?(standard8)
Chris, I've push some new commits to your branch. You now need to:

- Go through the diffs in the PR & remove all the logging statements.
- Rebase on latest master to pick up the Travis fix.
- Get Dan to finish reviewing.

Here's a summary of what I did:

- The existing infobar tests were too interleaved with the other tests that were running:

I split out to a new test file, started off by working out the basics - what tests were required (just like we do with unit tests), and then filling in the tests themselves. I realised that working with start/stopBrowserSharing direct was a much more consistent and easier way of testing, as that's where we are actually controlling the sidebar.

- Moved the listeners for room join/left from MozLoopService to bootstrap: start/stopBrowserSharing

I was confused as to why MozLoopService was trying to control the sharing. After taking the code out & realising what broke, it became obvious that the start/stopBrowserSharing should be setting up listeners direct, and it be handled alongside the rest of that code.

- The other 3 commits were minor tidy up & comments.
Comment on attachment 8726901 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-based-on-link-clicker-in-the-room-or-not > mozilla:master

Removed console logs and debugs. local mochitests passed except error posted in PR.
Attachment #8726901 - Flags: review?(dmose)
r=dmose in the PR, once the commits I've mentioned are added.
I've seen some odd test failures locally, but they are ones that I've also seen without these patches.  That said, I think this wants a try run before we land it.
Attachment #8726901 - Flags: review?(dmose) → review+
New try build to handle the alert:alert issues on non-Mac

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adca033ddabd
Try passed. I've already collapsed the commits into one.
https://github.com/mozilla/loop/commit/87bda15463cb9a948e7798b78fd1a24b5f51b92b
Status: ASSIGNED → RESOLVED
Iteration: --- → 48.3 - Apr 25
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 1265865
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: