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

[stable17] Do not load Talk sidebar in public share page of folder shares #2340

Conversation

danxuliu
Copy link
Member

Backport of #2339

Requires nextcloud/server#17597

Fixes #2282
Fixes #2293

Note that the Files_Sharing::loadAdditionalScripts event only includes the share since Nextcloud 17.0.1, so it is needed to handle the event without parameters to keep compatibility with Nextcloud 17.0.0 (in that case the Talk sidebar will still be loaded for folder shares).

@danxuliu danxuliu added 3. to review bug feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Oct 21, 2019
@danxuliu danxuliu added this to the 💙 Next Minor (17) milestone Oct 21, 2019
*/
public static function loadTalkSidebarUi() {
public static function loadTalkSidebarUi(?IShare $share) {
if ($share && $share->getNodeType() !== FileInfo::TYPE_FILE) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to switch to "hide by default".
This way we can help people to update their Nextcloud, as well as not allow to load the app on folders while the owner can not load the conversation in his file list?

Suggested change
if ($share && $share->getNodeType() !== FileInfo::TYPE_FILE) {
if (!$share || $share->getNodeType() !== FileInfo::TYPE_FILE) {

Copy link
Member Author

Choose a reason for hiding this comment

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

The Talk sidebar was loaded on folders, but it was not shown, so that was not a problem. Anyway, if you want to help people to update their Nextcloud, fine by me.

@danxuliu danxuliu force-pushed the backport/2339/stable17-do-not-load-talk-sidebar-in-public-share-page-of-folder-shares branch from 258a6f1 to e1bd518 Compare October 21, 2019 14:02
@nickvergessen
Copy link
Member

Conflicting files
lib/PublicShare/TemplateLoader.php

The Talk sidebar is only shown for file shares, so there is no need to
load it for folder shares. Moreover, this also prevents some of the
hacks used to show the Talk sidebar to mess with the layout used for
folders.

Note that the "Files_Sharing::loadAdditionalScripts" event only includes
the share since Nextcloud 17.0.1, so it is needed to handle the event
without parameters to keep compatibility with Nextcloud 17.0.0 (in that
case the Talk sidebar will not be loaded in any public share page until
the server is updated to 17.0.1 or later).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the backport/2339/stable17-do-not-load-talk-sidebar-in-public-share-page-of-folder-shares branch from e1bd518 to cf3eea4 Compare October 22, 2019 11:20
@danxuliu
Copy link
Member Author

Rebased due to conflicts with #2342.

@nickvergessen nickvergessen merged commit 7ad8ba1 into stable17 Oct 22, 2019
@delete-merged-branch delete-merged-branch bot deleted the backport/2339/stable17-do-not-load-talk-sidebar-in-public-share-page-of-folder-shares branch October 22, 2019 15:34
@FrankMaute
Copy link

Well, we use talk everyday. Please list it as a bug in order to get it solved. It was not addressed in RC1 of 17 which is a bit annoying.

@nickvergessen
Copy link
Member

Hi @FrankMaute It is not fixed in RC1, because Talk is not bundled with the server package.
But we will release a new version for Talk as well this week which should then come with this fix.

@FrankMaute
Copy link

Sounds really great. Thanks for the support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants