Align Context Menus

[luke-]

It would be good if we could somehow use the context menu from the Stream Entry with the options in the Cfiles module as well.

Stream Menu

File Menu

Probably there is a similar need in other places (e.g. Task module).

So probably it would be good if we create the possibility here in the core repo.

[yurabakhtin]

@luke-

It would be good if we could somehow use the context menu from the Stream Entry with the options in the Cfiles module as well.

Do I understand you correctly: you want to extend the cfiles context menu with additional options from stream menu, i.e. we should add options:

• Topics
• Change to Public
• Lock comments
• Turn off notifications
• Move content
• Move to archive

right?

[luke-]

@yurabakhtin Yes. It would be good if it is somehow the same (extended) menu. So if e.g. a module (e.g. Bookmarks) adds an entry option, this should also be available in the CFiles module automatically.

[yurabakhtin]

@luke- What I could implement in PR #131 and also small changes are required from core PR humhub/humhub#5261:

But there are still errors on click on some of the menu entries because really all they are implemented firstly to work from wall entry under widget stream.StreamEntry, so my implementation looks like a hack here https://github.com/humhub/cfiles/pull/131/files#diff-1e2c78de692f3804fd3363931f7ad1f6c388896340f73385de108d7f799cc87fR4 where I just added a wrapper <div data-ui-widget="stream.StreamEntry">.

The main difference between stream wall entry menu and cfiles context menu is that wall entry menu is intialized per each entry as separate html tag <ul> but for cfiles we have a single <ul> that is inialized same for all rows and the action there is called by JS code.

So current version still has errors because after each click on the menu entry we should reload a file row completely as it is done on wall stream entry side, because we need the reloading in order to update state of the menu entries, e.g. "Save as bookmark" vs "Remove from bookmarks". Currently I just blocked the reloading here https://github.com/humhub/humhub/pull/5261/files#diff-9b40039e0bb6ca8e0806ba5ea1c0dda8dde291b8fcebb7010a0c7cc104555833R91 that not good of course, but otherwise JS error.
I.e. we should somehow to find a solution for similar reloading with displaying message in state like "Content successfully removed from bookmarks." and update the context menu, but it is possible only by some additional ajax request because it is rendered on server side and we cannot change this there because it is in external modules like we have in "Content Bookmark".

Should I continue to implement this or do you think this is so complex and no need this feature?

[luke-]

@yurabakhtin I think you are already on a good way here. But I think we need to refactor a bit more/deeper to get a nice solution that also works in other modules.

What do you think about introduce new class ContentControls which is extended by WallEntryControls and only hold menu entries which are not requiring the Stream Wall Entry logic?

Do you also see a change to then extend FileListContextMenu from this ContentControls widget?

So that we end up with the following inheritances:

Widget -> Menu -> ContentControls -> WallEntryControls
Widget -> Menu -> ContentControls -> FileEntryControls

We can then move the entries from WallEntryControls to ContentControls step by step.

[yurabakhtin]

@luke- I don't see a reason to refactor PHP part, we need to change only JS core code as I started here https://github.com/humhub/humhub/pull/5261/files#diff-9b40039e0bb6ca8e0806ba5ea1c0dda8dde291b8fcebb7010a0c7cc104555833R91. I.e. we just need to modify the reload function to do this not only with wall entry but with others like rows from the cfiles browse table, I think it is easier way than we will redo each wall entry control in each module. Do you agree?

[luke-]

@yurabakhtin Somehow it doesn't look clean to me. It is somehow a mix of a NonMenu widget with a Menu widget.

We can leave out the separation between ContentControls and WallEntryControls if it is not necessary.

Is there a reason why the FileListContextMenu does not extends WallEntryControls and adds the file related things as MenuEntry.

---

Another example would be this here in the Tasks Module: - How would the code look like if I also wanted to use the WallEntryControlsMenu here with an additional MenuEntry in the first place?

[yurabakhtin]

@luke-

Is there a reason why the FileListContextMenu does not extends WallEntryControls and adds the file related things as MenuEntry.

I cannot know what reason was to implement this but yes I could agree with you that we can try to extend the FileListContextMenu from WallEntryControls, but after that we should create new html template for the FileListContextMenu and also rewrite JS code because currently all click actions from the FileListContextMenu works through JS here https://github.com/humhub/cfiles/blob/master/resources/js/humhub.cfiles.js#L132-L159.

Probably it was done as optimization to keep less html code on a page. As I wrote above the difference between stream wall and cfiles browse pages is:

• stream wall html page structure:

<div data-ui-widget="stream.StreamEntry"> // 1st wall Entry
    <ul><li>...</li>...</ul> // WallEntryControls
    // Wall entry content
<div>

<div data-ui-widget="stream.StreamEntry">  // Nth wall Entry
    ...
<div>

• cfiles browse html page structure:

<table>
    <tr> // 1st cfile
        <td>
            File title
            <ul><li>...</li>...</ul> // PLACE A: WallEntryControls (I added this recently because they should be initialized per object then on show context menu action this menu is appended to the FileListContextMenu below)
        </td>
        <td></td>
    <tr>

    <tr> // Nth cfile
    </tr>
</table>

// PLACE B: FileListContextMenu:
<div data-ui-widget="stream.StreamEntry"> // <-- please note this is a dirty code, I just added this recently in order to hide many errors on click from WallEntryControls because they require a parent stream.StreamEntry
    <ul class="contextMenuFolder"><li>...</li>...</ul>
    <ul class="contextMenuFile"><li>...</li>...</ul>
    <ul class="contextMenuImage"><li>...</li>...</ul>
    <ul class="contextMenuAllPostedFiles"><li>...</li>...</ul>
</div>

So as you can see the FileListContextMenu is rendered same once for all cfile rows.
I think yes we can try to extend the FileListContextMenu from WallEntryControls and then render new modified FileListContextMenu at the PLACE A and remove PLACE B of course, but it requires to rewrite JS code where all click actions are coded of the FileListContextMenu.

Should I start to modify the FileListContextMenu to an extended from WallEntryControls?

---

• How would the code look like if I also wanted to use the WallEntryControlsMenu here with an additional MenuEntry in the first place?

I have reviewed the TaskContextMenu and looks almost similar to WallEntryControls, so it can be easier extended with WallEntryControls than FileListContextMenu.

[luke-]

@yurabakhtin Thanks for the info.

Yes, please migrate the FileListContextMenu so that it extends WallEntryControls.

I think this is the best way for the the long run, if such menus clearly depend on the Menu classes and there is no merge of different appraches.

I am still considering whether we should not also introduce ContentControls. Then we could perhaps remove a few WallEntry only links such as "Pin".

[yurabakhtin]

@luke- The migration FileListContextMenu is in process.

[yurabakhtin]

@luke- I have committed first version of the extended FileListContextMenu here dc14ee9 but it is not works well yet, I need to change JS code to reload row after update like it works on wall stream side.

[luke-]

@yurabakhtin ok, thanks for the update

[yurabakhtin]

@luke- New update in the commit 7c6b224, to reload the file row after some stream action, but not all actions still work there, e.g. "Pin" and "Archive" doesn't work.

[yurabakhtin]

@luke- In today's commits of these 2 PRs https://github.com/humhub/cfiles/pull/131/commits and https://github.com/humhub/humhub/pull/5261/commits I have fixed all Js errors, so currently all context menu entries works well. They may be merged.

I am still considering whether we should not also introduce ContentControls. Then we could perhaps remove a few WallEntry only links such as "Pin".

To remove the "Pin" entry I just call a code $this->renderOptions->disableControlsEntryPin(); from FileListContextMenu side. I didn't implement ContentControls yet because as I wrote it would not solve all JS issues what I fixed today. We might implement the ContentControls only for better code order and if you think it will does development easier in future, however we can disable each menu entry from cfiles side as I wrote above. Let me know if I should start this refactoring.

---

Please note the 2 PRs have broken checks because of error:

Exception 'yii\base\InvalidArgumentException' with message 'The file or directory to be published does not exist: /home/runner/work/cfiles/cfiles/protected/vendor/npm-asset/humhub-prosemirror-richtext/dist'

I have missed the dist folder today's morning too when humhub-prosemirror-richtext was updated to 1.1.6 version, but then after updating to 1.1.7 the folder appeared as expecting, so probably there is same issue on the server.

[luke-]

@yurabakhtin Thanks looks good. Can you please see my review?

Regarding the test errors, can you try to update the develop branch in this PR?

[yurabakhtin]

@luke- I have done new changes for your remarks - eda8f38.

[luke-]

@yurabakhtin Here another small UI improvement

[yurabakhtin]

@luke-

Remove: Can you please add this option to "Move" Dialog as "Move to another Space"

Sorry, not sure what you mean here, probably it was already implemented here #84, if yes, then as I got I just need to implement the same for folders.

Remove: Not supported by CFiles

Why is it not supported, I see it works well as from wall stream side as from cfiles browser page:

Only one issue we can see here is that the dividers are hidden when the content is archived, it is because in core code the DropdownDivider::class is disabled for all archived entries: https://github.com/humhub/humhub/blob/master/protected/humhub/modules/content/widgets/stream/WallStreamEntryWidget.php#L165:

->disableControlsEntry(DropdownDivider::class);

I think it should be fixed somehow, either from core side or from CFiles module side.

---

I have detected one issue and fixed in the commit 593d525 of the same PR:

[luke-]

@yurabakhtin Regarding "Move":

It is not clear where the difference between "Move" and "Move content" is.

So we should remove "Move content" and add a link here:

[luke-]

@yurabakhtin Regarding "Archive":

Even if the "Archive" link is working, it has no effect on the view except for the status message.
This is confusing for many users.

In the Stream view also the Wall Entry disappears.

So the best solution would be at the moment:

CFiles View - Context Menu: Don't show Archive Link
Stream Entry CFiles Context Menu: Show Archive Link as before

This can probably easily done by a flag, or?

[yurabakhtin]

@luke- Thank you for the details with screenshot, I have done all in commit dbec8e7, it works like this:

---

CFiles View - Context Menu: Don't show Archive Link

Ok, I have hidden that.

This can probably easily done by a flag, or?

I think it would be good if we will mark the archived files with some grey background on the files browser page, so in such case user will knows the files is not visible on the wall stream side.

[luke-]

@yurabakhtin Thanks, looks good!

Regarding "Archive" I've added a seperate issue #135 - we may optimize this in the future.