Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend shared worker #3276

Merged
merged 52 commits into from
Apr 5, 2024
Merged

Conversation

bastianjoel
Copy link
Member

@bastianjoel bastianjoel commented Feb 13, 2024

resolves #3248
resolves #3259
resolves #3401

This PR involves a lot of cleanup in the shared worker. Also tests were added.

TODO:

  • Autoupdate
    • Add tests to autoupdate part
    • Rework unexpected stream resolve handling (session termination, restart)
    • Fix issues with stream pausing
    • Longpolling
  • ICC
    • Implement icc protocol in shared worker
    • Update icc adapter
  • Auth via shared worker

@bastianjoel bastianjoel self-assigned this Feb 13, 2024
@bastianjoel bastianjoel added this to the 4.3 milestone Feb 28, 2024
@bastianjoel bastianjoel added feature enhancement General enhancement which is neither bug nor feature labels Feb 28, 2024
@bastianjoel bastianjoel force-pushed the icc-to-shared-worker branch from 68a387e to a9ac43e Compare March 4, 2024 14:44
@luisa-beerboom
Copy link
Member

luisa-beerboom commented Mar 12, 2024

Using accessibility modifiers for constructors is not consistently done in the client. I don't like introducing this rule right now with this PR.

It is not an introduction, it is overwhelmingly done in the client for the simple reason that it is an established convention to put visibility modifiers on everything. The exceptions where it is inconsistent are merely instances that were not caught during the code review.

I am not asking you to fix it everywhere right now, I am asking you to fix it in your code

@luisa-beerboom luisa-beerboom removed their assignment Mar 12, 2024
Copy link
Member

@luisa-beerboom luisa-beerboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or fix this in another PR

@bastianjoel bastianjoel force-pushed the icc-to-shared-worker branch from 6ece271 to fa485bc Compare March 19, 2024 10:33
@bastianjoel bastianjoel force-pushed the icc-to-shared-worker branch from fa485bc to 2e033fe Compare March 19, 2024 10:34
@bastianjoel
Copy link
Member Author

@emanuelschuetze @MSoeb @Elblinator @rrenkert Maybe it is not completely obvious by the title and description but this PR includes the longpolling implementation and is finished since last week.
If anybody is feeling qualified to test this feel free to add a review.

The performance tool has a new tool broken-proxy to test this.

Copy link
Member

@rrenkert rrenkert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected.

But the warning sign is not centered:

image

@rrenkert rrenkert assigned bastianjoel and unassigned rrenkert Apr 3, 2024
@bastianjoel
Copy link
Member Author

bastianjoel commented Apr 3, 2024

>:(

@bastianjoel
Copy link
Member Author

This is a problem with the banner service. Offline mode and out of sync have the same problem. For that reason I think this is totally unrelated to this PR. Please open another issue.

@Elblinator
Copy link
Member

I created a new Issue
see #3506

@Elblinator Elblinator assigned rrenkert and unassigned bastianjoel Apr 5, 2024
@Elblinator Elblinator requested a review from rrenkert April 5, 2024 10:48
@Elblinator Elblinator dismissed rrenkert’s stale review April 5, 2024 11:50

change requests were moved to another Issue

@rrenkert rrenkert merged commit c268c6f into OpenSlides:main Apr 5, 2024
3 checks passed
@bastianjoel bastianjoel deleted the icc-to-shared-worker branch April 5, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancement which is neither bug nor feature feature
Projects
None yet
6 participants