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

Do not pool /notifications when the topbar is not visible #138

Open
SamuAlfageme opened this issue Feb 14, 2018 · 7 comments
Open

Do not pool /notifications when the topbar is not visible #138

SamuAlfageme opened this issue Feb 14, 2018 · 7 comments

Comments

@SamuAlfageme
Copy link

Dumping the contents of owncloud/core#30385 (comment)

I noticed that regular pooling to the /notifications endpoint is also happening under the hood on an open OAuth's "authorize" screen.

As a rule of thumb, I'd say when the top bar (where the notification container is located) is hidden, we should simply not pool this endpoint for events. I'll create an issue.

@PVince81
Copy link
Contributor

@jvillafanez can you take care ?

@jvillafanez
Copy link
Member

I guess we'll go with the easiest solution available.
I think I know what's happening and I probably have an easy solution even though it's technically awful to the point that santa won't give me Christmas presents this year. 🤷‍♂️

@jvillafanez
Copy link
Member

Options:

  • Blacklist urls (as it's being done now). As easy as awful.
  • Changes in core to provide a way to register apps in the top bar, similar to what the settings page does. As said, this requires changes in core that need to be evaluated.

@PVince81
Copy link
Contributor

How about just not loading this notifications JS file when on a page that is not the main layout ? (ex: public page, login page, etc)

@jvillafanez
Copy link
Member

jvillafanez commented Feb 16, 2018

That's mostly the second option.
Right now, core doesn't provide anything similar to "load this script / css / whatever in the main layout" as far as I know. This means that the script will be loaded always, which is bad. Then it's the script responsability to detect if it's the main layout or not (or any other kind of detection) to start polling.

The current option checks the requested URL in PHP to decide to load or not the script file, which leads to this issue.

@PVince81
Copy link
Contributor

Maybe we could also simply add some CSS classes on the html element of the different layout templates in a way that makes them distinguishable ? The values in question could even be read by the OC Javascript API, for example OC.getLayoutName() ?

@jvillafanez
Copy link
Member

If the page is handled by core, it's better to let core provide the tools required for the apps to hook in. Basically, core will fetch whatever is needed from the app.
It isn't the app the one that needs to hack its way, but core the one that should provide whatever the app needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants