Conversation
|
@RickStanley thanks for the contribution. Will look when the build passes; can you look into it? |
|
I've made some changes, sorry forgot to run tests and lint. I don't know if I can pick this up later, but I'm thinking about using the dialog element instead of the current Furthermore, we may need some tests for this. |
No worry 🙂.
Seems like a good idea for accessibility indeed. Up to you, if you have the bandwidth to improve this now in this PR, go for it. But if not, I'd rather have this PR merged now and a later PR improving the a11y, than this PR lingering while the dialog/browserwindow take 2 never comes.
Yes please! |
|
Finally! Build passing :). I'm not sure how to test this. Could you give me directions so I can take a look when I'm not busy? It may take some time, so I can't tell for sure when I'll be able to get my hands on this. As for dialog element/browserwindow for screen share picker: I think the latter is the better option, on the long run, thinking about support and web standards of dialog, maybe it's not ready yet (my opinion). And create an issue as an enhancement. |
This prevents the scenario where the promise never finishes.
|
Seems not working with these changes. |
🎉 🎉 🎉 🎉 !!!
@RickStanley well I'm not sure either 😄. In the end it's mostly plumbing and there isn't much to test on our side 🤷 , it's difficult-to-test "integration" / "end-to-end" stuff. 🤔 actually I have an idea: could you add a test at the end of our manual-test script that I run before new releases? You should add a extra manual test block that follows the structure established by the other tests: open a login-less page (maybe on Jitsi?) and offer a list of bullet points to enable a smoke-tester / maintainer to quickly whtaever test points { X, Y, Z } all pass.
👍
@DimICE thx for the notice; letting @RickStanley follow up with you. |
|
Closing as abandoned. Feel free to rebase, address comments, and add a comment, and I'll be happy to re-review. |
I'm picking up @RickStanley's abandoned PR #1321 again to add screensharing support (fixes #927), with the following additional changes: - In newer Electron versions, `desktopCapturer.getSources` must be called from the main process, so I solved this with an IPC call. - Importing from `./helpers/helpers` in 'preload.ts' does not work, as was mentioned by @DimICE in #1321 (comment). I'm not very familiar with TypeScript or Electron, so not sure why that is and how it could be solved - for now I just copied the referenced `isWayland` function to `preload.ts`. - Add a screensharing test to the manual test script, as requested by @ronjouch in #1321 (comment) As far as I understood from the discussion in #1321, the last point was basically the only thing that was missing to get this merged, correct? --------- Co-authored-by: Rick Stanley <rick-stanley@outlook.com> Co-authored-by: Rick Stanley <rick.stanley@lambda3.com.br> Co-authored-by: Ronan Jouchet <ronan@jouchet.fr>
This will, in theory, fix #927 . I made this from a new environment, my personal computer, where I don't use teams, so it's not tested yet, but it should work, as I've made these changes on my work laptop and worked flawlessly.
Some notes:
setupSessionPermissionHandler) is, in my opinion, a bad thing. Electron is not "safe by default", and force permission to get this working is not a good idea... Unfortunately we don't have another option as of this moment. Also see: Disallow permission requests by default electron/electron#12931;Screen share icon mentioned in the last item from the previous list:
