Skip to content

feat: Allow user to control message view mode #10945

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: Allow user to control message view mode #10945

wants to merge 5 commits into from

Conversation

SebastianKrupinski
Copy link
Contributor

@SebastianKrupinski SebastianKrupinski commented Mar 29, 2025 •
edited
Loading

Resolves: #5913

Summary

  • Adds the ability for a user to control message viewing mode (Threaded View or Singleton View)
  • Adds the ability for a administrator to preset default message viewing mode (Threaded View or Singleton View)
  • Changes behavior of drag and dropping message(s) to folder depending on viewing mode (Threaded View or Singleton View)
  • Changes behavior of Message List -> Message Actions menu (Move, Delete, Snooze) depending on viewing mode (Threaded View or Singleton View)

User Setting

Screenshot 2025-03-29 164914

Administrator Settings

Screenshot 2025-03-29 164801

@SebastianKrupinski SebastianKrupinski self-assigned this Mar 29, 2025
@github-project-automation github-project-automation bot moved this to 🏗️ In progress in 💌 📅 👥 Groupware team Mar 29, 2025
@SebastianKrupinski SebastianKrupinski force-pushed the enh/issue-5913-diable-mail-threading branch from ae8adf6 to 97d05cd Compare March 29, 2025 21:32
@SebastianKrupinski
feat: Allow disabling of threaded view
2c6772c 
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
@SebastianKrupinski SebastianKrupinski force-pushed the enh/issue-5913-diable-mail-threading branch from 97d05cd to 2c6772c Compare March 29, 2025 21:38
@ChristophWurst
Copy link
Member

If you receive two messages that form a thread, you will still only see one entry in your INBOX with the current approach, right?

@GretaD
Copy link
Contributor

GretaD commented Apr 3, 2025

before
1

after
2

GretaD
GretaD approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@GretaD GretaD left a comment

Choose a reason for hiding this comment

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

it works as described

ChristophWurst
ChristophWurst requested changes Apr 3, 2025
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Tested and doesn't work. It's only possible to open the latest received message of a thread. I can not open earlier ones, because they are not shown in the message list. The list is still threaded.

@SebastianKrupinski
Copy link
Contributor Author

Tested and doesn't work. It's only possible to open the latest received message of a thread. I can not open earlier ones, because they are not shown in the message list. The list is still threaded.

Correct! I was not aware we stacked messages in the list, I have already made the backed and front end changes, just need to updated some backend tests, will push the update soon.

@SebastianKrupinski
fixup! feat: Allow disabling of threaded view
3d6cd5e 
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
@SebastianKrupinski
fixup! feat: Allow disabling of threaded view
8498a7a 
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
@SebastianKrupinski
Copy link
Contributor Author

Tested and doesn't work. It's only possible to open the latest received message of a thread. I can not open earlier ones, because they are not shown in the message list. The list is still threaded.

Corrected, message are no longer stacked in message list.

image

ChristophWurst
ChristophWurst reviewed Apr 7, 2025
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks sane

Are there any plans to skip building threads if the user doesn't use them anyway?

@SebastianKrupinski
fixup! feat: Allow disabling of threaded view
0e91868 
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
ChristophWurst
ChristophWurst reviewed Apr 7, 2025
View reviewed changes
lib/Controller/MessagesController.php Outdated
@@ -124,6 +124,8 @@ public function __construct(string $appName,
* @param int $cursor
* @param string $filter
* @param int|null $limit
* @param string $sort returns messages in requested order ('newest' or 'oldest')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param string $sort returns messages in requested order ('newest' or 'oldest')
* @param IMailSearcH::sort_* $sort

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't use IMailSearcH::sort_* here the UI sends 'newest' or 'oldest' and the value of IMailSearch::ORDER_* is 'DESC' and 'ASC'

Copy link
Member

Choose a reason for hiding this comment

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

And this can't be streamlined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was existing code for the most part, the only change to the sort was that the UI now sends the sort order vs the controller pulling the value from the database every time.

I didn't want to refactor sort as it was out of scope for this PR and would make the review more difficult.

I can address this in a follow-up if you want, or do you want it as part of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Do a follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, removed the sort code changes.

ChristophWurst
ChristophWurst reviewed Apr 15, 2025
View reviewed changes
lib/Controller/MessagesController.php
if ($view !== null) {
$view = $view === 'singleton' ? IMailSearch::VIEW_SINGLETON : IMailSearch::VIEW_THREADED;
}
$messages = $this->mailSearch->findMessages(
Copy link
Member

Choose a reason for hiding this comment

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

Inline the var and you have a smaller diff ✨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, Hopefully this is what you ment

lib/Controller/MessagesController.php Outdated
@@ -124,6 +124,8 @@ public function __construct(string $appName,
* @param int $cursor
* @param string $filter
* @param int|null $limit
* @param string $sort returns messages in requested order ('newest' or 'oldest')
Copy link
Member

Choose a reason for hiding this comment

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

Do a follow-up

@ChristophWurst
Copy link
Member

Are there any plans to skip building threads if the user doesn't use them anyway?

@SebastianKrupinski?

@SebastianKrupinski
fixup! feat: Allow disabling of threaded view
63b7546 
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
@SebastianKrupinski
Copy link
Contributor Author

Are there any plans to skip building threads if the user doesn't use them anyway?

@SebastianKrupinski?

Fixed, took some major head scratching to find the correct code.

@SebastianKrupinski
Copy link
Contributor Author

Will fix lint errors on final approval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GretaD GretaD GretaD approved these changes

@st3iny st3iny Awaiting requested review from st3iny

@GVodyanov GVodyanov Awaiting requested review from GVodyanov

@kesselb kesselb Awaiting requested review from kesselb kesselb is a code owner

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst ChristophWurst is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@SebastianKrupinski SebastianKrupinski

Labels
3. to review
Projects
💌 📅 👥 Groupware team
Status: 🏗️ In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable Mail Threading
3 participants
@SebastianKrupinski @ChristophWurst @GretaD