New message rendering methods on IMAP #155

paulyoung · 2013-06-26T03:38:48Z

I'm creating a pull request so it's easier to discuss and track changes as opposed to in the issues themselves.

As discussed in #112 and #111, this is a first pass at defining the interface. Currently only includes IMAP. RFC 822 to come.

I need to look further into the implementation and how the existing IMAPMessage::htmlRendering and HTMLRenderer::htmlForIMAPMessage methods work. It's unclear to me at this point if these new methods need to take additional parameters for the callbacks or if callbacks will be created within their implementation.

includes IMAP. RFC 822 to come. * HTML rendering - MailCore#112 * Plain text - MailCore#111

paulyoung · 2013-06-27T04:59:48Z

For the asynchronous methods, what would the return type be?

dinhvh · 2013-06-27T05:03:37Z

It would be void but and a callback would be provided.

class IMAPMessageRenderingCallback {
fetchFinished(IMAPAsyncSession *session, IMAPMessage, ErrorCode error, String * result);
};

The asynchronous method has no return type (void) and will return immediately.
The callback will be called when the result is ready.

paulyoung · 2013-06-27T05:05:11Z

Where would the callback class live? Is it OK to put it in MCIMAPAsyncSession or should it have it's own file? (I'm not sure of your conventions)

dinhvh · 2013-06-27T05:12:25Z

I just though about cancellation.
-> Think about fast scrolling in a UITableView.
The message content would load but would not be valid when the callback is called.

Maybe we should not be providing an Operation. That would make it less simple to use but would cover that case properly.

paulyoung · 2013-06-27T05:15:37Z

Maybe we should not be providing an Operation.

What would the alternative be?

dinhvh · 2013-06-27T05:17:29Z

I meant "we should be providing an operation".
I though about making simpler but does not solve the cancel issue. We might have similar issue with objc.

Hoa V. Dinh

On Wednesday, June 26, 2013 at 10:15 PM, Paul Young wrote:

Maybe we should not be providing an Operation.

What would the alternative be?

—
Reply to this email directly or view it on GitHub (#155 (comment)).

paulyoung · 2013-06-27T05:20:25Z

So mailcore::IMAPHTMLRenderingOperation for example?

dinhvh · 2013-06-27T05:24:43Z

IMAPMessageRenderingOperation. Create 4 methods but one operation class that change its behavior using flags.

paulyoung · 2013-06-27T05:28:13Z

Nice.

* Revert commit 22b05c3. * Revert commit 35d29ed. * Revert commit db2671b. * Revert commit af57027.

paulyoung · 2013-06-27T05:36:34Z

What should the targets for the new file be? (For the build settings)

dinhvh · 2013-06-27T05:38:59Z

"static mailcore2 ios" and "static mailcore2 osx".

paulyoung · 2013-06-27T06:29:46Z

Hopefully this is a start. I tried to understand how other operations work and what they're doing.

A nudge in the right direction from here would be good.

dinhvh · 2013-06-27T06:37:46Z

The start would be to implement the synchronous methods in IMAPSession.
By using It should be fairly easy by using:
class IMAPMessage {
virtual String * htmlRendering(String * folder,
HTMLRendererIMAPCallback * dataCallback,
HTMLRendererTemplateCallback * htmlCallback = NULL);
}

You have to create an object that implements HTMLRendererIMAPCallback and make the call to IMAPSession when in the callbacks.

* Added rendering enumerated type. * Added result string. * Removed data. * Moved rendering methods from IMAP message rendering operation to IMAP session.

paulyoung · 2013-06-27T21:53:58Z

By using It should be fairly easy by using:

class IMAPMessage {
virtual String * htmlRendering(String * folder,
HTMLRendererIMAPCallback * dataCallback,
HTMLRendererTemplateCallback * htmlCallback = NULL);
}

I don't follow exactly what you mean by this. I think you're saying make a call to this method, since it appears to already exist.

dinhvh · 2013-06-27T21:54:45Z

Yes, you should make a call to htmlRendering(). It perform all the work for you.

* Passing message ivar to methods in `main()`.

* Created a generic message renderer helper class. * Renamed "msg" to "message" in session rendering methods.

paulyoung · 2013-06-29T04:30:03Z

Would appreciate some guidance on this. I'm a little unclear on how to actually implement the callback methods. I'm guessing I'll need more than one of each in order to return just the body when necessary.

dinhvh · 2013-06-29T04:31:51Z

You should just override the templateformainheader method to return an empty string.

Hoa V. Dinh

On Friday, June 28, 2013 at 9:30 PM, Paul Young wrote:

Would appreciate some guidance on this. I'm a little unclear on how to actually implement the callback methods. I'm guessing I'll need more than one of each in order to return just the body when necessary.

—
Reply to this email directly or view it on GitHub (#155 (comment)).

paulyoung · 2013-06-29T04:38:18Z

You should just override the templateformainheader method to return an empty string.

But I only want to return an empty string when only the body is requested. Not sure if you're suggesting having a separate (sub)class for rendering only the body, or using a single class and putting a conditional inside that method.

dinhvh · 2013-07-11T17:02:05Z

From master (https://github.com/MailCore/mailcore2), by using those instructions, I got a conflict in the xcodeproj file:

git checkout -b paulyoung-message-rendering-with-session master
git pull https://github.com/paulyoung/mailcore2.git message-rendering-with-session

paulyoung · 2013-07-11T17:11:01Z

Do you have local changes on master that haven't been pushed?

paulyoung · 2013-07-11T17:11:50Z

I don't mean unstaged changes, I mean commits. Is your local branch ahead of the remote?

dinhvh · 2013-07-11T17:17:22Z

I'm locally in sync with github.
Did you try the commands I described?

Hoà V. Dinh

On Thursday, July 11, 2013 at 10:11 AM, Paul Young wrote:

I don't mean unstaged changes, I mean commits. Is your local branch ahead of the remote?

—
Reply to this email directly or view it on GitHub (#155 (comment)).

paulyoung · 2013-07-11T18:04:27Z

Yeah, it was a fast-forward merge with no conflicts.

paulyoung · 2013-07-12T00:08:34Z

I'm getting the following error when trying to implement in the examples.

Receiver type 'MCOIMAPMessageRenderingOperation' for instance message is a forward declaration.

How can I make sure that the class is available?

dinhvh · 2013-07-12T00:30:13Z

Include it in MCOIMAP.h and in targets "static mailcore osx" and "static mailcore ios", add this file to "copy files" phase.

paulyoung · 2013-07-12T01:44:19Z

I think this means I have too many calls "release" (I'm assuming MC_SAFE_RELEASE).

2013-07-11 21:41:56.968 [49285:4303] MCIMAPSession.cc:2293: had error : 0
2013-07-11 21:41:56.975 [49285:4303] MCObject.cc:63: release too much 0x7d73f30 mailcore::String
/Users/Paul/git/mailcore2/build-mac/../src/core/basetypes/MCObject.cc:64: assert 0

dinhvh · 2013-07-12T01:50:32Z

What's the crash stack trace?

paulyoung · 2013-07-12T01:52:40Z

Let me know if you need more info.

dinhvh · 2013-07-12T01:55:36Z

src/async/imap/MCIMAPMessageRenderingOperation.cc

+    }
+    else if (mRenderingType == IMAPMessageRenderingTypePlainTextBody) {
+        mResult = session()->session()->plainTextBodyRendering(mMessage, folder(), &error);
+    }


mResult is not retained. It might be the cause.

paulyoung · 2013-07-12T02:48:14Z

This works end to end now. I'm happy to make any further changes/optimizations.

I'd need to understand how testing works to add those.

dinhvh · 2013-07-12T02:56:24Z

We can merge this first part for now.
Whenever you feel confident, remove the "NOT READY TO MERGE" at the top, then, we're good to go.

paulyoung · 2013-07-12T03:07:34Z

I'm going to sync with master before we merge.

…-with-session Conflicts: build-mac/mailcore2.xcodeproj/project.pbxproj

paulyoung · 2013-07-12T03:13:35Z

Good to merge. 👍

Simple message rendering methods on IMAP.

pushkarsingh · 2013-07-21T17:17:43Z

(MCOIMAPMessageRenderingOperation *) plainTextRenderingOperationWithMessage:(MCOIMAPMessage *)message
folder:(NSString *)folder;

How do we construct MCOIMAPMessage from the data we have saved? I might have only uid,subject, headers etc from the previous fetch of messages, I could not find a easy way to construct MCOIMAPMessage from that.

dinhvh · 2013-07-21T19:04:47Z

For 0.3, I plan to make MCOIMAPMessage serializable.
For now, if you just have the UID, you can fetch it using:

 MCOIMAPFetchMessagesOperation * op = [session fetchMessagesByUIDOperationWithFolder:@"INBOX"
                                                                         requestKind:MCOIMAPMessagesRequestKindHeaders | MCOIMAPMessagesRequestKindStructure
                                                                                uids:MCORangeMake(uid, uid)];
 [op start:^(NSError * error, NSArray * messages, MCOIndexSet * vanishedMessages) {
    // messages array will contain 0 or 1 MCOIMAPMessage.
 }];

paulyoung added 4 commits June 25, 2013 23:28

First pass at interface for new rendering methods. This currently only

5164e89

includes IMAP. RFC 822 to come. * HTML rendering - MailCore#112 * Plain text - MailCore#111

Revert commit 5164e89.

255c552

Added interface for synchronous methods.

af57027

Added newline.

db2671b

Added error code parameter. Followed existing conventions.

35d29ed

Added interface for asynchronous methods.

22b05c3

Revert changes to IMAP sessions.

9a0e635

* Revert commit 22b05c3. * Revert commit 35d29ed. * Revert commit db2671b. * Revert commit af57027.

First pass at IMAP message rendering operation.

ee860ed

Updated message rendering operation.

1c723f5

* Added rendering enumerated type. * Added result string. * Removed data. * Moved rendering methods from IMAP message rendering operation to IMAP session.

paulyoung added 3 commits June 28, 2013 23:15

Made plain text body the default rendering type.

2d13ec9

Replaced UID ivar and accessor methods with equivalents for message.

3dafb66

* Passing message ivar to methods in `main()`.

WIP: Message Renderer Helper

7ee045b

* Created a generic message renderer helper class. * Renamed "msg" to "message" in session rendering methods.

paulyoung added 2 commits July 11, 2013 19:43

Added missing include.

9762e9d

Added file to build targets.

eb59c8d

paulyoung added 2 commits July 11, 2013 21:27

Added missing import.

f334e3e

Added missing call to set message on rendering operation.

6d9f8a8

dinhvh reviewed Jul 12, 2013
View reviewed changes

paulyoung added 2 commits July 11, 2013 22:22

Retaining message ivar.

80aa893

Cnsolidated calls to release message ivar.

90f9e57

paulyoung mentioned this pull request Jul 12, 2013

Implement message preview in the examples. #132

Closed

Merge remote-tracking branch 'upstream/master' into message-rendering…

cf54644

…-with-session Conflicts: build-mac/mailcore2.xcodeproj/project.pbxproj

paulyoung mentioned this pull request Jul 12, 2013

Add message preview to examples. #191

Merged

dinhvh added a commit that referenced this pull request Jul 12, 2013

Merge pull request #155 from paulyoung/message-rendering-with-session

df96333

Simple message rendering methods on IMAP.

dinhvh merged commit df96333 into MailCore:master Jul 12, 2013

paulyoung deleted the message-rendering-with-session branch July 12, 2013 04:49

proforov added a commit to proforov/mailcore2_deprecated that referenced this pull request Aug 13, 2014

libetpan updated for issue MailCore#155

e116674

New message rendering methods on IMAP #155

New message rendering methods on IMAP #155

Conversation

paulyoung commented Jun 26, 2013

paulyoung commented Jun 27, 2013

dinhvh commented Jun 27, 2013

paulyoung commented Jun 27, 2013

dinhvh commented Jun 27, 2013

paulyoung commented Jun 27, 2013

dinhvh commented Jun 27, 2013

paulyoung commented Jun 27, 2013

dinhvh commented Jun 27, 2013

paulyoung commented Jun 27, 2013

paulyoung commented Jun 27, 2013

dinhvh commented Jun 27, 2013

paulyoung commented Jun 27, 2013

dinhvh commented Jun 27, 2013

paulyoung commented Jun 27, 2013

dinhvh commented Jun 27, 2013

paulyoung commented Jun 29, 2013

dinhvh commented Jun 29, 2013

paulyoung commented Jun 29, 2013

dinhvh commented Jul 11, 2013

paulyoung commented Jul 11, 2013

paulyoung commented Jul 11, 2013

dinhvh commented Jul 11, 2013

paulyoung commented Jul 11, 2013

paulyoung commented Jul 12, 2013

dinhvh commented Jul 12, 2013

paulyoung commented Jul 12, 2013

dinhvh commented Jul 12, 2013

paulyoung commented Jul 12, 2013

dinhvh Jul 12, 2013

Choose a reason for hiding this comment

paulyoung commented Jul 12, 2013

dinhvh commented Jul 12, 2013

paulyoung commented Jul 12, 2013

paulyoung commented Jul 12, 2013

pushkarsingh commented Jul 21, 2013

dinhvh commented Jul 21, 2013