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

Status: RESOLVED FIXED

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

Comment 1

[Romain Testard [:RT]]

Matej could you please help us here with appropriate copies for the scenarios mentionned above?

Comment 2

[Matej Novak [:matej]]

(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."

Comment 3

[Romain Testard [:RT]]

(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.

Comment 4

[Matej Novak [:matej]]

(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."

Comment 5

[Romain Testard [:RT]]

Oops, now fixed, my mistake!

Comment 6

[Chris Rafuse :crafuse]

create bug to remove the following string after new strings are implemented:
infobar_screenshare_browser_message2

Comment 7

[GitHub Autolander Bot]

Attachment: [loop] chrafuse:bug-1239972-adapt-infobar-message-if-link-clicker-in-room-or-not > mozilla:master

Comment 8

[Chris Rafuse :crafuse]

Comment on attachment 8717520 [details] [review]
[loop] chrafuse:bug-1239972-adapt-infobar-message-if-link-clicker-in-room-or-not > mozilla:master

Strings section.

Comment 9

[Dan Mosedale (:dmosedale, :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

Comment 10

[Chris Rafuse :crafuse]

Starting infobar guest in room status message change.

Comment 11

[Romain Testard [:RT]]

Updating priority to reflect the fact we want this one done in our sprint commencing this week.

Comment 12

[Chris Rafuse :crafuse]

Attachment: 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.

Comment 13

[Chris Rafuse :crafuse]

Attachment: Unpaused Empty Room

Comment 14

[Chris Rafuse :crafuse]

Comment on attachment 8726866 [details]
Unpaused Empty Room

Here is message for Un-paused in an empty room.

Comment 15

[Chris Rafuse :crafuse]

Attachment: 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.

Suggestions of future live tab sharing connotations:

https://www.google.ca/imgres?imgurl=https://cleeng.com/blog/wp-content/uploads/2016/01/live-broadcast.jpg&imgrefurl=https://cleeng.com/blog/going-ott-broadcast-live-key-challenges/&h=448&w=817&tbnid=O1DITanB9qqtdM:&docid=7rjqU7MSzfl7AM&ei=fN_ZVtquFpjcjwOx24K4DA&tbm=isch&ved=0ahUKEwja3P3S36fLAhUY7mMKHbGtAMcQMwhjKD8wPw

-or-

https://www.google.ca/imgres?imgurl=https://www.airtime.pro/wp-content/uploads/2015/01/live-broadcast-airtime1.jpg&imgrefurl=https://www.airtime.pro/tag/live-broadcast/&h=427&w=640&tbnid=PW3VuG2JK4Wv2M:&docid=hh1eB8ps0zTAQM&ei=fN_ZVtquFpjcjwOx24K4DA&tbm=isch&ved=0ahUKEwja3P3S36fLAhUY7mMKHbGtAMcQMwhFKCEwIQ#h=427&imgdii=PW3VuG2JK4Wv2M%3A%3BPW3VuG2JK4Wv2M%3A%3BjUJxH2_c-EJqwM%3A&w=640

Comment 16

[Chris Rafuse :crafuse]

Attachment: Paused Guest In Room and not sharing tabs.

Paused state and guest in room and not sharing tabs.

Comment 17

[Sevaan Franks [:sevaan]]

(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 18

[Sevaan Franks [:sevaan]]

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?

Comment 19

[GitHub Autolander Bot]

Attachment: [loop] chrafuse:bug-1239972-adapt-infobar-message-based-on-link-clicker-in-the-room-or-not > mozilla:master

Comment 20

[Chris Rafuse :crafuse]

Attachment: Paused Empty Room with updated string

Updated string.

Comment 21

[Chris Rafuse :crafuse]

Attachment: Paused Guest In Room

Added punctuation and will need translation.

Comment 22

[Chris Rafuse :crafuse]

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.

Comment 23

[Dan Mosedale (:dmosedale, :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

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.

Comment 24

[Dan Mosedale (:dmosedale, :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.

Comment 25

[Dan Mosedale (:dmosedale, :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.

Comment 26

[Chris Rafuse :crafuse]

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?

Comment 27

[Mark Banner (: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.

Comment 28

[Chris Rafuse :crafuse]

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.

Comment 29

[Dan Mosedale (:dmosedale, :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.

Comment 30

[Chris Rafuse :crafuse]

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.

Comment 31

[Dan Mosedale (:dmosedale, :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.

Comment 32

[Chris Rafuse :crafuse]

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?

Comment 33

[Mark Banner (:standard8)]

Commented in the PR.

Comment 34

[Mark Banner (: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 35

[Chris Rafuse :crafuse]

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.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

r=dmose in the PR, once the commits I've mentioned are added.

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 38

[Mark Banner (:standard8)]

Try build happening at:

https://hg.mozilla.org/try/rev/cd8587e81a60d531160a846181504dbac70796f2

Comment 39

[Mark Banner (:standard8)]

New try build to handle the alert:alert issues on non-Mac

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adca033ddabd

Comment 40

[Mark Banner (:standard8)]

Try passed. I've already collapsed the commits into one.

Comment 41

[Mark Banner (:standard8)]

https://github.com/mozilla/loop/commit/87bda15463cb9a948e7798b78fd1a24b5f51b92b