Always on top #1861

hristoterezov · 2017-08-04T13:06:22Z

No description provided.

virtuacoplenny · 2017-08-09T16:28:13Z

react/features/toolbox/functions.web.js

+ * @returns {boolean} - True to indicate that the given toolbar button
+ * is enabled, false - otherwise.
+ */
+export function isButtonEnabled(name) {


What did this change of moving the function provide you? I'm not against it but I also don't understand why you moved it.

I'm guessing now you did it for a lighter package?

I refactored a little bit UIUtils in order to decrease the size of always on top bundle. Specifically isButtonEnabled was a good candidate to be moved in the toolbox feature. You think it is better to be defined in UIUtils or somewhere else?

virtuacoplenny · 2017-08-09T16:29:31Z

react/features/toolbox/components/ToolbarButton.web.js

                        button.tooltipText,
                        tooltipPosition);
                } else {
-                    UIUtil.setTooltip(this.button,
+                    setTooltip(this.button,


Why did you move tooltips? Again, I'm not against this but I don't understand what it gives you, especially with reactification of tooltips in review.

I started this as a part of the effort to reduce the size of the always on top bundle. But at the end the problem with UIUtils turned out to be related to an unused import. I don't think I need this change but at the end I liked how the tooltips are separated from all other random util methods.

I think I can easily revert change if needed?

I was not aware of the tooltips reactification status but I don't see a problem with this. Are you worried that this changes are going to be in conflict with the reactification PR?

virtuacoplenny · 2017-08-09T16:31:35Z

react/features/toolbox/components/StatelessToolbar.web.js

+ * toolbar items such as buttons. Toolbar is commonly placed inside of a
+ * Toolbox.
+ *
+ * @class Toolbar


I think these comments need updating, at least redoing the "class Toolbar" part.

virtuacoplenny · 2017-08-09T16:36:05Z

react/features/toolbox/components/ToolbarButton.web.js

                { this._renderPopups(popups) }
-            </a>
+            </StatelessToolbarButton>


I don't fully understand the naming of "stateless." Could you explain to me why the button/toolbar on master is not stateless? When I think "stateful" I think of something with this.state.

I named it "stateless" because it doesn't use this.state, lifecycle methods and redux.

Okay, so stateless as in like a react stateless component, one with just a render method?

virtuacoplenny · 2017-08-09T16:43:19Z

conference.js

@@ -2343,6 +2345,7 @@ export default {
            'device count: ' + videoDeviceCount);

        APP.store.dispatch(setVideoAvailable(available));
+        APP.API.notifyVideoAvailabilityChanged(available);


I'm wondering these API calls are better to do in redux/actions/middleware. However, it would make the logic more difficult to detect while conference.js is still in heavy use. Is it tricky to move these calls out of conference and into the react/redux app world?

I suppose it won't be tricky but personally I would prefer to move the API module first. Anyway it will be another task.

virtuacoplenny · 2017-08-09T16:55:33Z

react/features/always-on-top/AlwaysOnTop.js

+            api.isAudioMuted(),
+            api.isVideoMuted(),
+            api.isAudioAvailable(),
+            api.isVideoAvailable()


I just noticed a naming difference between isXEnabled vs isXAvailable. Do the mean the same thing? What does "enabled" mean?

They are absolutely the same. Initially I was using enabled for everything because this was the naming in jitsi-meet. But after that when I was rebasing the branch everything was changed to available. Probably I forgot to rename the events. Will fix it!

virtuacoplenny · 2017-08-09T18:06:53Z

react/features/always-on-top/AlwaysOnTop.js

+import StatelessToolbarButton
+    from '../toolbox/components/StatelessToolbarButton';
+
+const { api } = window.alwaysOnTop;


Where does window.alwaysOnTop get defined?

jitsi-meet-electron-utils project defines it.

virtuacoplenny · 2017-08-10T15:44:56Z

doc/api.md

@@ -141,6 +141,20 @@ The `listener` parameter is a Function object with one argument that will be not

 The following events are currently supported:

+* **audioEnabledStatusChanged** - event notifications about audio enabled status changes. The listener will receive an object with the following structure:


Does this event name still get fired?

No. I forgot to rename it here. Thanks for pointing it! I'll fix it!

virtuacoplenny · 2017-08-10T15:45:22Z

doc/api.md

@@ -196,6 +210,20 @@ changes. The listener will receive an object with the following structure:
 }
 ```

+* **videoEnabledStatusChanged** - event notifications about video enabled status changes. The listener will receive an object with the following structure:


Does this event name still get fired?

No. I forgot to rename it here. Thanks for pointing it! I'll fix it!

virtuacoplenny · 2017-08-10T16:24:32Z

modules/API/API.js

+            return false;
+        }
+
+        return true;


What does the return true/false do exactly? I see it used elsewhere too.

true means that the message is processed and false means that it is not processed. If a message is not processed we are caching the message. You can see the logic in Transport.emit.

virtuacoplenny · 2017-08-10T16:33:37Z

react/features/toolbox/components/ToolbarButton.web.js

                { this._renderPopups(popups) }
-            </a>
+            </StatelessToolbarButton>


Okay, so stateless as in like a react stateless component, one with just a render method?

virtuacoplenny · 2017-08-10T16:39:11Z

react/features/always-on-top/AlwaysOnTop.js

+    }
+
+    /**
+     * Removes the mouse move listener.


I think this comment needs updating because more than the mouse move listener is removed.

virtuacoplenny · 2017-08-10T16:42:11Z

react/features/always-on-top/AlwaysOnTop.js

+        )
+        .catch(console.error);
+
+        window.addEventListener('mousemove', this._mouseMove);


mousemove will cause a lot of events to get captured. There was no problem encountered?

I haven't noticed any problems. Aren't we doing the same thing for jitsi-meet's toolbar? Do you have something in mind to improve it?

@saghul

fix(ProxyConnection) set signaling layer by @saghul in jitsi#1862 fix: Fixes version while building. by @damencho in jitsi#1866 feat(multi-stream-support)Add support for multiple local and remote tracks/mediaType. by @jallamsetty1 in jitsi#1861 fix: Add private message handler for breakout_rooms by @steve-vsee in jitsi#1867 fix(JingleSessionPC) Do not force track removal at pc level on user leave. by @jallamsetty1 in jitsi#1869 feat(multi-stream-support) Handle SDP munging for multiple local/remote streams per ep. by @jallamsetty1 in jitsi#1868

@saghul

fix(ProxyConnection) set signaling layer by @saghul in #1862 fix: Fixes version while building. by @damencho in #1866 feat(multi-stream-support)Add support for multiple local and remote tracks/mediaType. by @jallamsetty1 in #1861 fix: Add private message handler for breakout_rooms by @steve-vsee in #1867 fix(JingleSessionPC) Do not force track removal at pc level on user leave. by @jallamsetty1 in #1869 feat(multi-stream-support) Handle SDP munging for multiple local/remote streams per ep. by @jallamsetty1 in #1868

@saghul

fix(ProxyConnection) set signaling layer by @saghul in jitsi#1862 fix: Fixes version while building. by @damencho in jitsi#1866 feat(multi-stream-support)Add support for multiple local and remote tracks/mediaType. by @jallamsetty1 in jitsi#1861 fix: Add private message handler for breakout_rooms by @steve-vsee in jitsi#1867 fix(JingleSessionPC) Do not force track removal at pc level on user leave. by @jallamsetty1 in jitsi#1869 feat(multi-stream-support) Handle SDP munging for multiple local/remote streams per ep. by @jallamsetty1 in jitsi#1868

hristoterezov force-pushed the alwaysontop branch from 459a967 to 48a60e9 Compare August 4, 2017 13:11

hristoterezov mentioned this pull request Aug 4, 2017

feat(alwaysontop): Implement jitsi/jitsi-meet-electron#17

Merged

hristoterezov added 8 commits August 7, 2017 11:48

feat(external_api): Return external resources for the always on top.

ef4ce23

ref(ToolbarButton): Remove dispatch

c58b1a1

ref(isButtonEnable): UIUtil -> toolbox

dc6255b

ref(UIUtil): Move all tooltip functions into another file

8e31059

fix(ToolbarButton): Don't register shortcut if APP is not defined.

16439e8

ref(ToolbarButton): Remove unnecessary check.

566e10d

ref(toolbar): Implement stateless toolbar

e2ac34d

fix(postmessagebackend): JS error when response is sent.

a9fdace

hristoterezov force-pushed the alwaysontop branch 3 times, most recently from 292d03f to dafa93a Compare August 7, 2017 15:17

saghul self-requested a review August 8, 2017 14:30

virtuacoplenny self-assigned this Aug 9, 2017

virtuacoplenny reviewed Aug 9, 2017

View reviewed changes

hristoterezov force-pushed the alwaysontop branch from dafa93a to 752beb6 Compare August 10, 2017 12:58

virtuacoplenny approved these changes Aug 10, 2017

View reviewed changes

feat(alwaysontop): Toolbar.

88435c5

hristoterezov force-pushed the alwaysontop branch from 752beb6 to 88435c5 Compare August 11, 2017 08:51

virtuacoplenny merged commit 1782030 into master Aug 12, 2017

bgrozev deleted the alwaysontop branch October 27, 2017 18:50

jallamsetty1 mentioned this pull request Feb 7, 2022

chore(deps) lib-jitsi-meet@latest. #10922

Merged

This was referenced Apr 5, 2022

update ljm 1649173091 #11319

Closed

update ljm 1649316923 #11331

Closed

update ljm 1649325751 #11336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always on top #1861

Always on top #1861

hristoterezov commented Aug 4, 2017

virtuacoplenny Aug 9, 2017

virtuacoplenny Aug 9, 2017

hristoterezov Aug 10, 2017

virtuacoplenny Aug 9, 2017

hristoterezov Aug 10, 2017 •

edited

Loading

virtuacoplenny Aug 9, 2017

virtuacoplenny Aug 9, 2017

hristoterezov Aug 10, 2017

virtuacoplenny Aug 10, 2017

hristoterezov Aug 11, 2017

virtuacoplenny Aug 9, 2017

hristoterezov Aug 10, 2017

virtuacoplenny Aug 9, 2017

hristoterezov Aug 10, 2017

virtuacoplenny Aug 9, 2017

hristoterezov Aug 10, 2017

virtuacoplenny Aug 10, 2017

hristoterezov Aug 11, 2017

virtuacoplenny Aug 10, 2017

hristoterezov Aug 11, 2017

virtuacoplenny Aug 10, 2017

hristoterezov Aug 11, 2017

virtuacoplenny Aug 10, 2017

virtuacoplenny Aug 10, 2017

virtuacoplenny Aug 10, 2017

hristoterezov Aug 11, 2017

		@@ -141,6 +141,20 @@ The `listener` parameter is a Function object with one argument that will be not

		The following events are currently supported:

		* audioEnabledStatusChanged - event notifications about audio enabled status changes. The listener will receive an object with the following structure:

Always on top #1861

Always on top #1861

Conversation

hristoterezov commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hristoterezov Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hristoterezov Aug 10, 2017 •

edited

Loading