After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 738187 - Allow select-replies through message menu
Allow select-replies through message menu
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: composer
master
Other Linux
: Normal normal
: ---
Assigned To: Geary Maintainers
Geary Maintainers
review
Depends on:
Blocks:
 
 
Reported: 2014-10-08 22:21 UTC by Robert Schroll
Modified: 2014-10-24 12:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow select-reply through message menu (2.34 KB, patch)
2014-10-20 03:30 UTC, Robert Schroll
needs-work Details | Review
Allow select-reply through message menu (3.54 KB, patch)
2014-10-21 03:23 UTC, Robert Schroll
committed Details | Review

Description Robert Schroll 2014-10-08 22:21:28 UTC
In bug #712912, we introduced select-replies, but only through the header bar button and the shortcut.  This was not enabled for replies through the message menu.  This was triggered by considering the case where text in one message is selected and "reply" is activated via another message's menu.  Obviously, select-reply shouldn't happen then, and it was decided to never select-reply from these menus to avoid confusion.

But on reconsideration, I don't think there's much danger of confusion.  We should allow select-replies through this message if the selection is in the same message.
Comment 1 Robert Schroll 2014-10-20 03:30:00 UTC
Created attachment 288876 [details] [review]
Allow select-reply through message menu

This leaves us with a bit of duplication between the on_*_message() and 
on_*_message_action() methods.  I tried to combine them into one method 
with a nullable callback, but Vala wouldn't accept that as an acceptible 
callback for the action.
Comment 2 Jim Nelson 2014-10-20 22:32:41 UTC
Review of attachment 288876 [details] [review]:

Could this be refactored something like this?

on_reply_to_message(message) {
  create_reply_forward_widget(ComposeType.REPLY, message);
}

on_reply_all_message(message) {
  create_reply_forward_widget(ComposeType.REPLY_ALL, message);
}

on_forward_message() {
  // you get the idea
}

create_reply_forward_widget(compose-type, message) {
  // get selected message, get quote, then call create_compose_widget
}

I'd be fine then replacing these 3 on_ calls with closures as signal handlers, if that makes sense.

I think the on_*_message_action calls could be refactored as well using a similar technique.  But I don't think all six could be refactored to use a single common helper method without some additional flags that would probably overcomplicate the problem.
Comment 3 Robert Schroll 2014-10-21 03:23:42 UTC
Created attachment 289002 [details] [review]
Allow select-reply through message menu

It turns out that it's fairly easy to send all six calls through the 
same path.  I've added a commit explaining what's going on at the 
beginning; I can also try to explain the logic if you think it's not 
obvious.

If I were writing this from scratch, I'd probably use closures, but I 
don't think there's much of an advantage in replacing the method calls 
now.
Comment 4 Jim Nelson 2014-10-23 19:31:06 UTC
Review of attachment 289002 [details] [review]:

Very nice.  Commit!
Comment 5 Robert Schroll 2014-10-24 12:41:35 UTC
Attachment 289002 [details] pushed as da881c3 - Allow select-reply through message menu