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.

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

[ENH]message list: Preview of message when viewing message list #1241

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[ENH]message list: Preview of message when viewing message list #1241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions modules/core/message_list_functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,15 +295,23 @@ function checkbox_callback($vals, $style, $output_mod) {
if (!hm_exists('subject_callback')) {
function subject_callback($vals, $style, $output_mod) {
$img = '';
$preview_msg = '';
$subject = '';
if (count($vals) == 4 && $vals[3]) {
$img = '<i class="bi bi-filetype-'.$vals[3].'"></i>';
}
$subject = $output_mod->html_safe($vals[0]);
if (is_array($vals[0])) {
$preview_msg = $output_mod->html_safe($vals[0]['preview_msg']);
$subject = $output_mod->html_safe($vals[0]['subject']);
} else {
$subject = $output_mod->html_safe($vals[0]);
}

$hl_subject = preg_replace("/^(\[[^\]]+\])/", '<span class="s_pre">$1</span>', $subject);
if ($style == 'news') {
return sprintf('<div class="subject"><div class="%s" title="%s">%s <a href="%s">%s</a></div></div>', $output_mod->html_safe(implode(' ', $vals[2])), $subject, $img, $output_mod->html_safe($vals[1]), $hl_subject);
return sprintf('<div class="subject"><div class="%s" title="%s">%s <a href="%s">%s</a><p class="fw-light" title="'. $preview_msg .'">'. $preview_msg .'</p></div></div>', $output_mod->html_safe(implode(' ', $vals[2])), $subject, $img, $output_mod->html_safe($vals[1]), $hl_subject);
}
return sprintf('<td class="subject"><div class="%s"><a title="%s" href="%s">%s</a></div></td>', $output_mod->html_safe(implode(' ', $vals[2])), $subject, $output_mod->html_safe($vals[1]), $hl_subject);
return sprintf('<td class="subject"><div class="%s"><a title="%s" href="%s">%s</a><p class="fw-light" title="'. $preview_msg .'">'. $preview_msg .'</p></div></td>', $output_mod->html_safe(implode(' ', $vals[2])), $subject, $output_mod->html_safe($vals[1]), $hl_subject);
}}

/**
Expand Down
9 changes: 7 additions & 2 deletions modules/imap/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,11 @@ function format_imap_message_list($msg_list, $output_module, $parent_list=false,
$msg['subject'] = '[No Subject]';
}
$subject = $msg['subject'];
$preview_msg = "";
if (isset($msg['preview_msg'])) {
$preview_msg = $msg['preview_msg'];
}

if ($parent_list == 'sent') {
$icon = 'sent';
$from = $msg['to'];
Expand Down Expand Up @@ -291,7 +296,7 @@ function format_imap_message_list($msg_list, $output_module, $parent_list=false,
$res[$id] = message_list_row(array(
array('checkbox_callback', $id),
array('icon_callback', $flags),
array('subject_callback', $subject, $url, $flags, $icon),
array('subject_callback', array("subject" => $subject, "preview_msg" => $preview_msg), $url, $flags, $icon),
Copy link
Member

Choose a reason for hiding this comment

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

subject_callback is under your control - please add a separate parameter there for preview_msg instead of using subject and preview in an array - it is harder to understand what $vals[0] mean in the callback. Better use separate named parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted

array('safe_output_callback', 'source', $source),
array('safe_output_callback', 'from'.$nofrom, $from, null, str_replace(array($from, '<', '>'), '', $msg['from'])),
array('date_callback', $date, $timestamp),
Expand All @@ -307,7 +312,7 @@ function format_imap_message_list($msg_list, $output_module, $parent_list=false,
array('checkbox_callback', $id),
array('safe_output_callback', 'source', $source, $icon),
array('safe_output_callback', 'from'.$nofrom, $from, null, str_replace(array($from, '<', '>'), '', $msg['from'])),
array('subject_callback', $subject, $url, $flags),
array('subject_callback', array("subject" => $subject, "preview_msg" => $preview_msg), $url, $flags),
array('date_callback', $date, $timestamp, $is_snoozed),
array('icon_callback', $flags)
),
Expand Down
9 changes: 9 additions & 0 deletions modules/imap/handler_modules.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,14 @@ public function process() {
$msg['server_id'] = $form['imap_server_id'];
$msg['server_name'] = $details['name'];
$msg['folder'] = $form['folder'];
$uid = $msg['uid'];
$part = true;
$max = 87;
$msg_struct = $imap->get_message_structure($uid);
$struct = $imap->search_bodystructure($msg_struct, array('imap_part_number' => $part));
$msg_struct_current = array_shift($struct);
$msg_text = $imap->get_message_content($uid, $part, $max, $msg_struct_current);
$msg['preview_msg'] = $msg_text;
christer77 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@kroky kroky Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The way you do it has several problems:

  1. Too many internal calls to imap method.
  2. Too many calls to imap server for each message - it will be rather slow for bigger lists of messages. Might slow down the normal list pages by 2-4 times.
  3. I think we should seriously do this conditionally based on a setting that user turns on and off for specific pages (imap folder pages, combined pages).
  4. The code to get the preview should be moved out of here as this only works for single imap folder pages. We have many other places listing messages (search results, combined pages - all, sent, unread, etc.)
  5. It is best to have a setting, pass it to the imap get_mailbox_page or get_message_list methods and have it query the text body as well if required. This will ensure we get all the content in a single call to the imap server. Please test this with 50 messages at once and you will notice the difference. Check if the IMAP protocol doesn't have something built-in - like getting part of the text body of the message or getting only the text body, not the whole structure. Parsing the bodystructure is an expensive operation. If we really need to do this for each message separately in list-view, we should consider aggressive caching.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted

$msgs[] = $msg;
}
if ($imap->selected_mailbox) {
Expand Down Expand Up @@ -2191,3 +2199,4 @@ function process_move_messages_in_screen_email_enabled_callback($val) { return $
process_site_setting('move_messages_in_screen_email', $this, 'process_move_messages_in_screen_email_enabled_callback', true, true);
}
}

2 changes: 1 addition & 1 deletion modules/imap/site.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1378,7 +1378,7 @@ var add_email_in_contact_trusted = function(list_email) {
}
);
}
};
};

$('.screen-email-unlike').on("click", function() { imap_screen_email(); return false; });

Expand Down
4 changes: 2 additions & 2 deletions tests/phpunit/modules/core/message_list_functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ public function test_checkbox_callback() {
*/
public function test_subject_callback() {
$mod = new Hm_Output_Test(array('foo' => 'bar', 'bar' => 'foo'), array('bar'));
$this->assertEquals('<td class="subject"><div class=""><a title="foo" href="bar">foo</a></div></td>', subject_callback(array('foo', 'bar', array()), 'email', $mod));
$this->assertEquals('<div class="subject"><div class="" title="foo"><i class="bi bi-filetype-code"></i> <a href="bar">foo</a></div></div>', subject_callback(array('foo', 'bar', array(), 'code'), 'news', $mod));
$this->assertEquals('<td class="subject"><div class=""><a title="foo" href="bar">foo</a><p class="fw-light" title=""></p></div></td>', subject_callback(array('foo', 'bar', array()), 'email', $mod));
$this->assertEquals('<div class="subject"><div class="" title="foo"><i class="bi bi-filetype-code"></i> <a href="bar">foo</a><p class="fw-light" title=""></p></div></div>', subject_callback(array('foo', 'bar', array(), 'code'), 'news', $mod));
}
/**
* @preserveGlobalState disabled
Expand Down