Folder management

[smartmobili]

Hi,

I am looking at folder management inside kairos and it's really messy.
There is one part inside SMMailAccount.j where we can find methods
like :

- (SMMailBox)createMailbox:(id)sender
- (void)removeMailbox:(SMMailbox)aMailbox
...

and another part inside SMMailBox.j with methods like :

- (void)renameTo:(CPString)aName
- (void)remove
...

And those methods generally calls methods from SMMailAccount.j.
But what's the point ? Why is it so complex ?

About the fact that you create a renameOrCreateFolder because you
didn't know if it was a creation or a rename operation I don't think
this is a good idea. For the moment (maybe it will be necessary one
day) I would prefer to be able to separate and clearly identify those
operations.
I am quite sure that when we choose new folder inside the context menu
or when we click on the + icon we could set a member variable like
_creationMode and pass this information to some other method.

About renameOrCreateFolder maybe instead of returning null or string I
would prefer something more general because I am not even sure we can
return null for object different from String.
And sometimes we may want to pass more information than just a string
about the object state, what about the following prototype :

def renameOrCreateFolder(oldName: String, destName: String):
SMMailbox = {
}
And add a member variable error to all remote objects.

But actually the returned value doesn't seem to be used, if I follow
the workflow I get this :

1) A user creates a new folder(clicking on + or using context menu)

- (IBAction)addMailbox:(id)sender (MailSourceViewController.j)
{
- (SMMailBox)createMailbox:(id)sender (inside this method we
create a new SMMailBox and we add it to the mailboxes)

we reload data source => it will update the ui and display a new
folder in edit mode with a default name

}

Then when user validate the new name for the folder I suppose the
following event will be triggered :

- (void)outlineView:(CPOutlineView)anOutlineView setObjectValue:
(id)aValue forTableColumn:(CPTableColumn)aColumn byItem:(id)anItem
{
[mailbox renameTo:aValue]; // The creation is really done
here
[self reload]; // once again we reload data source
}

With this logic we never hanlde the value returned by
renameOrCreateFolder.

I would prefer somehing like that (don't really know if it possible
but I am sure that a better solution than the current one exists)

- (IBAction)addMailbox:(id)sender
{
if (![self canAddMailbox])
return;

_creationMode = "create"

var newMailbox = [[mailController mailAccount]
createMailbox:self];
[self reload];
[self selectMailbox:newMailbox];

var view = [self view],
rowIndex = [[view selectedRowIndexes] firstIndex];
if (rowIndex !== nil && rowIndex !== CPNotFound)
[view editColumn:0 row:rowIndex withEvent:nil select:YES];
}

- (void)outlineView:(CPOutlineView)anOutlineView setObjectValue:
(id)aValue forTableColumn:(CPTableColumn)aColumn byItem:(id)anItem
{
var mailbox = [anItem object];
if (![mailbox isSpecial])
{
// The object SMMailBox has already been created inside the UI
so now
// that we have choosen its new name we want to really create
it on server
[mailbox save:aValue withCreationMode:_creationMode];


[self reload];
}
}

I am still not satisfied by what I propose but I need to think more
about it.

[Victor Kazarinov]

Vincent, 

Please find answers bellow inline:

I am looking at folder management inside kairos and it's really messy.
There is one part inside SMMailAccount.j where we can find methods
like :
- (SMMailBox)createMailbox:(id)sender
- (void)removeMailbox:(SMMailbox)aMailbox
...
and another part inside SMMailBox.j with methods like :
- (void)renameTo:(CPString)aName
- (void)remove
...
And those methods generally calls methods from SMMailAccount.j.
But what's the point ? Why is it so complex ?

The code is a little bit messy. But it is hierarchical structure, because one Account contains several boxes (folders). So when we need create MailBox (folder), we ask (call) method in MailAccount, which actually create new MailBox object. So Account (parent) manages boxes (childs). Also note that client is depend on the server. Note that both this classes (SMMailbox and SMMailAccount) is subclasses of SMRemoteObject, so that 2 classes also exists in code of the server side. So "messy" starts at server, and client uses that architecture of "accounts" and "boxes". 

About the fact that you create a renameOrCreateFolder because you
didn't know if it was a creation or a rename operation I don't think
this is a good idea. For the moment (maybe it will be necessary one
day) I would prefer to be able to separate and clearly identify those
operations.
I am quite sure that when we choose new folder inside the context menu
or when we click on the + icon we could set a member variable like
_creationMode and pass this information to some other method.

Right, I didn't know if it is renaming or creation, because current UI architecture actually asks server to create/rename only when folder name editing in UI is ended. This is how it was designed (renameTo methods and etc. was before I started to work with this code). And I was agree with this architecture, because actually we not need to create folder in IMAP at server, before user enters an name for it from keyboard. And actual creation is happens only when user ended editing text of folder name in UI. And this event also triggered not only when creation new folder, but also when renaming an existing folder, so this is why I created renameOrCreateFolder function. Bellow I see your suggestion with "_creationMode", I agree to separate renaming and creation.

About renameOrCreateFolder maybe instead of returning null or string I
would prefer something more general because I am not even sure we can
return null for object different from String.
And sometimes we may want to pass more information than just a string
about the object state, what about the following prototype :
 def renameOrCreateFolder(oldName: String, destName: String):
SMMailbox = {
 }
And add a member variable error to all remote objects.

In last commits I fixed that part and now it not return Null anymore, just strings. This is temporary solution, until we will make (and think up) an generic error returning solution. I agree, this should be some field (member variable) with error, an error code. It may be an integer variable or enum with list of errors, and on client side it can be converted into localized error description for user, for example.

But actually the returned value doesn't seem to be used, if I follow
the workflow I get this :
1) A user creates a new folder(clicking on + or using context menu)
- (IBAction)addMailbox:(id)sender (MailSourceViewController.j)
{
   - (SMMailBox)createMailbox:(id)sender (inside this method we
create a new SMMailBox and we add it to   the mailboxes)
   we reload data source => it will update the ui and display a new
folder in edit mode with a default name
}
Then when user validate the new name for the folder I suppose the
following event will be triggered :
- (void)outlineView:(CPOutlineView)anOutlineView setObjectValue:
(id)aValue forTableColumn:(CPTableColumn)aColumn byItem:(id)anItem
{
       [mailbox renameTo:aValue];  // The creation is really done
here
       [self reload];   // once again we reload data source
}
With this logic we never hanlde the value returned by
renameOrCreateFolder.

Return value is used. renameTo function in SMMailBox.j is call renameOrCreateFolder, and there is set selector @selector(imapServerDidRenameOrCreateFolder:). So when renameOrCreateFolder execution is ended at server, it calls back and it is used to show message with error for user (as we talked above, using error code as string is a temporary solution, because it not support localization and etc.):

- (void)imapServerDidRenameOrCreateFolder:(String)err 

{ 

    CPLog("imapServerDidRenameOrCreateFolder");

    if (err != "")

    {

        // Output error to user

        alert(err)

    }    

}

I understand what you mean with proposed solution: You added global flag "_creationMode"  to separate folder creation and folder renaming. I agree with that separation of creation and renaming functions. What you dislike in this solution? What do you think about it? 

I suggest to not touch "save" function and I suggest to use separate "createFolder" and "renameFolder" functions.

--
With best regards,
Victor Kazarinov

[smartmobili]

I have added Ignacio to this conversation even if I know that I might
not get any answer


By the way maybe _editMode or _updateMode would be a better name for
the variable or why not _upsertMode(for update or insert)

[mailbox save:aValue withEditMode:_editMode]
[mailbox save:aValue withUpsertMode:_upsertMode];

I have also tested to disable cache and I understand what you mean
when you say that it's really annoying everytime you select INBOX
folder because
all emails 'headers are downloaded again and again.
Finally it might also indicate that we need to use a cache
mechanism...
For information when testing on ubuntu, downloading email's headers is
not that slow (not more than 16 s).
I am also worried about the fact that we instanciate a HNRemoteService
too many times I think, wouln't be possible to have a kind of
singleton and always call the same instance ?
In code I can see the following comments :

// FIXME Seems like a waste to have one 'imapServer' instance per
mailbox, but every HNRemoteService
// can only have one, unchangable delegate.

But in this case why don't we get all imap responses inside the same
file ?

[smartmobili]

> I have added Ignacio to this conversation even if I know that I might
> not get any answer
>

Hum sorry I wanted to add Ignacio but at the end I didn't because
generally he doesn't answer and dont' want to bother him again
except we are stuck.

[Victor Kazarinov]

Hi Vincent,

No, there is no need cache. I made investigation if IMAP performance and found a bottleneck (one of it). It is a function "imapHeadersForFolder(…)" in ImapService.scala. It supposed to download all headers. Note, that we working with IMAP and it should download all requested headers with just one request to IMAP server. That imapHeadersForFolder(…) function currently makes a huge count of IMAP "FETCH" command requests to IMAP server, and this is why it is working slow, and it works more slowly with remote IMAP server, because of delay between client and server and every FETCH request takes time (delay).

I wrote small program in Java with using javamail library (mail-1.4.4.jar). It download headers from IMAP with just one FETCH request. It download headers for all mails from kairos test account in just 1 second!!  If you will try this code, don't forget to add javamail lib to the project, and fix password in "password_here" placeholder. 

Bellow the code there is more thoughts, please don't miss it.

Here is the code:

package test1pckg;

import java.util.*;

import javax.mail.*;

import com.sun.mail.imap.*;

public class StartUp {

/**

* @param args

*/

public static void main(String[] args) {

try {

        IMAPStore store = null;

        Session sessionIMAP = Session.getInstance(System.getProperties(), null);

        sessionIMAP.setDebug(true);

        try {

            store = (IMAPStore) sessionIMAP.getStore("imap");

            store.connect("mail.smartmobili.com",143,"webg...@smartmobili.com","password_here");

        } catch (Exception e) {

            e.printStackTrace();

        }

        IMAPFolder folder = (IMAPFolder) store.getFolder("INBOX");

        folder.open(Folder.READ_ONLY);

        System.out.println("start");

        Message[] msgs = folder.getMessages(1,folder.getMessageCount());

        long ftime = System.currentTimeMillis();

        FetchProfile fp=new FetchProfile();

        fp.add(FetchProfile.Item.ENVELOPE);

        fp.add("Newsgroups");

        folder.fetch(msgs, fp);

        long time = System.currentTimeMillis();

        String fetchTimeStr = "----!!!!!fetch time: "+(time-ftime) + " ms (1000ms = 1 sec)";

        System.out.println(fetchTimeStr);

        for (Message message : msgs) {

            System.out.println("Subject: " + message.getSubject());

          Address[] from = message.getFrom();

            for (Address address : from) {

                System.out.println("From: " + address);

            }

            Address[] recipients = message.getAllRecipients();

            for (Address address : recipients) {

                System.out.println("To: " + address);

            }

            System.out.println("--------");

        }

        long newTime = System.currentTimeMillis();

        System.out.println(fetchTimeStr);

        System.out.println("----!!!!output headers time: "+(newTime-time) + "ms (1000ms = 1 sec)");

    }catch (Exception e) {

        e.printStackTrace();

    }

}

}

So this code demonstration shows that it is possible to use IMAP without cache at all. Note that we download all headers, and in future we will download only 50 headers at once for page, so it should be even faster. And if we will create pools and keep connection opened, it will even faster, because it will not make LOGIN and LOGOUT commands to receive headers. But there is need to investigate and understand, perhaps there is reason why in imapHeadersForFolder(…) function is use another algorithm to receive headers, perhaps it require some special fields e.g. "DateTime" in special format and etc.

Perhaps there is more bottlenecks, not only in function imapHeadersForFolder(…).

 
I am also worried about the fact that we instanciate a HNRemoteService

too many times I think, wouln't be possible to have a kind of
singleton and always call the same instance ?

I agree, that this current implementation takes much memory and other resources in browser, so using singleton will be a solution for this. But I don't think that this task is primary. Also I don't understand what "delegate" parameter exactly mean in "HNRemoteService" class in function initForScalaTrait:

 imapServer = [[HNRemoteService alloc] initForScalaTrait:@"com.smartmobili.service.ImapService" objjProtocol:nil endPoint:nil delegate:self];

I know that this means that current class (object) (file) will receive events (e.g. callbacks) from HNRemoteService, which means it will receive events from server. But what exactly events? I don't see any (I don't know what to find). Because this code is part of Cardano, and I don't know what kind of events is expected to receive when we set delegate:self in "initForScalaTrait" function.

You asked:

But in this case why don't we get all imap responses inside the same
file ?

So again, as I described above, I don't know what kind of events to expect from  HNRemoteService. Perhaps we even don't catch them, because not require them. So the possible answer for your questions is: because we don't receive them at all in any file. When we call any function from "imapServer", e.g. [imapServer renameOrCreateFolder] we also set another delegate there to receive event callback. So, is it correlates in some way with delegate set in "initForScalaTrait" function above? If no, then we can use singleton, use one instance of "imapServer" and use delegates for functions in any other files, e.g. call "renameOrCreateFolder" not in same file where is initForScalaTrait was called. This is just my thoughts.

By the way maybe _editMode or _updateMode would be a better name for
the variable or why not _upsertMode(for update or insert)
 [mailbox save:aValue withEditMode:_editMode]
 [mailbox save:aValue withUpsertMode:_upsertMode];

I don't like to use "save" function, and like to use separate function to "create" folders, "rename" folders and etc. _editMode can be also an enum, it can have values "CreateFolder, RenameFolder" for example. By default is RenameFolder, and when user press button (+) to create folder, or press menu item to create folder, it can switch _editMode to "CreateFolder" and after creation switch back to RenameFolder.

[vincent richomme]

Hi Vincent,

No, there is no need cache. I made investigation if IMAP performance and found a bottleneck (one of it). It is a function "imapHeadersForFolder(…)" in ImapService.scala. It supposed to download all headers. Note, that we working with IMAP and it should download all requested headers with just one request to IMAP server. That imapHeadersForFolder(…) function currently makes a huge count of IMAP "FETCH" command requests to IMAP server, and this is why it is working slow, and it works more slowly with remote IMAP server, because of delay between client and server and every FETCH request takes time (delay).

I wrote small program in Java with using javamail library (mail-1.4.4.jar). It download headers from IMAP with just one FETCH request. It download headers for all mails from kairos test account in just 1 second!!  If you will try this code, don't forget to add javamail lib to the project, and fix password in "password_here" placeholder. 

Bellow the code there is more thoughts, please don't miss it.

 

Actually I wanted to talk about that because I also discovered what you are talking about.
I found it by loging imap commands from roundcube  (see https://github.com/smartmobili/kairos/blob/master/misc/docs/imap_logs/roundcube_simple).
I also think that since scala is a functional language it's easier to make that kind of error when working with data because we don't always
know what is hidden behind some methods like map.
But congratulations for your analysis.

 

[smartmobili]

I have also noticed that folder created with kairos doesn't appear
when using roundcube.
There is a notion of subscription because when roundcube asks for
folder's list it uses a different imap command :

[11-Jan-2012 14:22:56 +0100]: [18BE] C: A0003 LIST (SUBSCRIBED) "" "*"

Don't remember what imap command is used by kairos but it' different

On 11 jan, 20:15, vincent richomme <v.richo...@gmail.com> wrote:
> > Hi Vincent,
>
> > No, there is no need cache. I made investigation if IMAP performance and
> > found a bottleneck (one of it). It is a function "imapHeadersForFolder(…)"
> > in ImapService.scala. It supposed to download all headers. Note, that we
> > working with IMAP and it should download all requested headers with just
> > one request to IMAP server. That imapHeadersForFolder(…) function currently
> > makes a huge count of IMAP "FETCH" command requests to IMAP server, and
> > this is why it is working slow, and it works more slowly with remote IMAP
> > server, because of delay between client and server and every FETCH request
> > takes time (delay).
>
> > I wrote small program in Java with using javamail library
> > (mail-1.4.4.jar). It download headers from IMAP with just one FETCH
> > request. It download headers for all mails from kairos test account in just
> > 1 second!!  If you will try this code, don't forget to add javamail lib to
> > the project, and fix password in "password_here" placeholder.
> > Bellow the code there is more thoughts, please don't miss it.
>
> Actually I wanted to talk about that because I also discovered what you are
> talking about.

> I found it by loging imap commands from roundcube  (seehttps://github.com/smartmobili/kairos/blob/master/misc/docs/imap_logs...
> ).

[Victor Kazarinov]

I think after folder creation we should set folder to subscribed state
with setSubscribed command in IMAPFolder class. Here is doc:
http://javamail.kenai.com/nonav/javadocs/com/sun/mail/imap/IMAPFolder.html#setSubscribed(boolean)
Also doc says that not all IMAP servers support subscription.

Roundcube show only subscribed folders with listSubscribed command
from IMAP, here is doc:
http://javamail.kenai.com/nonav/javadocs/com/sun/mail/imap/IMAPFolder.html#listSubscribed(java.lang.String)

[Victor Kazarinov]

Hi Vincent,

I have done implementing folder creation with separate function to create folder from renaming folders. I used new _folderEditMode member variable and enum FolderEditModes. Please review latest commit ( https://github.com/smartmobili/kairos/commit/387cce93272a62076b886ddab752631ad9780b7d ) and let me know if you satisfied with current solution with using _folderEditMode and separate createFolder function. 

Current TODOs for folder creation/renaming:
1. Renaming of folders not yet implemented

2. Returned errors from folder creation function is still string (need to think out an standard for errors returning from ImapService to webapp), and this string is not using localization yet.

And I found the bug:

3. Bug: when renaming folder and after editing folder name user not press enter, but press mouse outside of edit field, event "setObjectValue" is not triggered, and so name of folder is not saved. Also here I see not usual behavior of edit field: when edit field is active, if we click to other item in list, edit field will disappear as expected (but  setObjectValue not triggered), but if we will click outside of list of folders, e.g. on white field of email content in kairos, then edit field of folder name will be with white text and will remain on screen, what is not expected behavior, and of course setObjectValue is also not triggered as expected, so folder name changes is not saved at server.

Right now I will work on folder renaming (1).

[vincent richomme]

2012/1/13 Victor Kazarinov <vic...@gmail.com>

Hi Vincent,

I have done implementing folder creation with separate function to create folder from renaming folders. I used new _folderEditMode member variable and enum FolderEditModes. Please review latest commit ( https://github.com/smartmobili/kairos/commit/387cce93272a62076b886ddab752631ad9780b7d ) and let me know if you satisfied with current solution with using _folderEditMode and separate createFolder function. 

Current TODOs for folder creation/renaming:
1. Renaming of folders not yet implemented

2. Returned errors from folder creation function is still string (need to think out an standard for errors returning from ImapService to webapp), and this string is not using localization yet.

And I found the bug:

3. Bug: when renaming folder and after editing folder name user not press enter, but press mouse outside of edit field, event "setObjectValue" is not triggered, and so name of folder is not saved. Also here I see not usual behavior of edit field: when edit field is active, if we click to other item in list, edit field will disappear as expected (but  setObjectValue not triggered), but if we will click outside of list of folders, e.g. on white field of email content in kairos, then edit field of folder name will be with white text and will remain on screen, what is not expected behavior, and of course setObjectValue is also not triggered as expected, so folder name changes is not saved at server.

Right now I will work on folder renaming (1).

So weird when testing on http://bifrost.smartmobili.com/kairos/capp I cannot reproduce (3).

Can you test it ?

 

[Victor Kazarinov]

Yes, it is reproducible. Note that there 2 bugs: one is that folder name changes is not saved, and other is that edit field is remains visible with white text. And I can't reproduce folder name changes (e.g. if setObjectValue is called or not) in http://bifrost.smartmobili.com/kairos/capp  because it seems there is used cache (?) so we can't see if folder name changes is saved or not. So it can be tested locally. Regarding 2nd bug, it is reproducible:

1. Click on an folder with double-click to start editing. 

2. Don't change name of folder

3. Click outside of left pane, for example on mail content at right down pane. You will see text field still there (this is bug) and font color is white (also bug).

Screenshots attached to this email (first is editing, and second if we click outside)

4. If we click back to editing field, it will looks like empty, but this is because font color is white now.

[Victor Kazarinov]

I figured out that first bug with setObjectValue is not a bug. It
works fine if we change name of folder. And it not trigger
setObjectValue event if folder name is not changed, so it is fine.
But second bug is real, and you can see it at "Screen Shot 2012-01-13
at 17.44.30.png" in previous email.

On Jan 13, 5:52 pm, Victor Kazarinov <vic...@gmail.com> wrote:
> Yes, it is reproducible. Note that there 2 bugs: one is that folder name
> changes is not saved, and other is that edit field is remains visible with
> white text. And I can't reproduce folder name changes (e.g.

> if setObjectValue is called or not) inhttp://bifrost.smartmobili.com/kairos/capp because it seems there is used

> cache (?) so we can't see if folder name changes is saved or not. So it can
> be tested locally. Regarding 2nd bug, it is reproducible:
> 1. Click on an folder with double-click to start editing.
> 2. Don't change name of folder
> 3. Click outside of left pane, for example on mail content at right down
> pane. You will see text field still there (this is bug) and font color is
> white (also bug).
> Screenshots attached to this email (first is editing, and second if we
> click outside)
> 4. If we click back to editing field, it will looks like empty, but this is
> because font color is white now.
>

> On Fri, Jan 13, 2012 at 5:40 PM, vincent richomme <v.richo...@gmail.com>wrote:
>
>
>
>
>
>
>
>
>
>
>
> > 2012/1/13 Victor Kazarinov <vic...@gmail.com>
>
> >> Hi Vincent,
>
> >> I have done implementing folder creation with separate function to create
> >> folder from renaming folders. I used new _folderEditMode member variable
> >> and enum FolderEditModes. Please review latest commit (

> >>https://github.com/smartmobili/kairos/commit/387cce93272a62076b886dda...) and let me know if you satisfied with current solution with using

> >> _folderEditMode and separate createFolder function.
>
> >> Current TODOs for folder creation/renaming:
> >> 1. Renaming of folders not yet implemented
> >> 2. Returned errors from folder creation function is still string (need to
> >> think out an standard for errors returning from ImapService to webapp), and
> >> this string is not using localization yet.
> >> And I found the bug:
> >> 3. Bug: when renaming folder and after editing folder name user not press
> >> enter, but press mouse outside of edit field, event "setObjectValue" is not
> >> triggered, and so name of folder is not saved. Also here I see not usual
> >> behavior of edit field: when edit field is active, if we click to other
> >> item in list, edit field will disappear as expected (but  setObjectValue
> >> not triggered), but if we will click outside of list of folders, e.g. on
> >> white field of email content in kairos, then edit field of folder name will
> >> be with white text and will remain on screen, what is not expected
> >> behavior, and of course setObjectValue is also not triggered as expected,
> >> so folder name changes is not saved at server.
>
> >> Right now I will work on folder renaming (1).
>
> >> So weird when testing onhttp://bifrost.smartmobili.com/kairos/cappI
> > cannot reproduce (3).
> > Can you test it ?
>
> --
> With best regards,
> Victor Kazarinov
>

>  Screen Shot 2012-01-13 at 17.44.25.png
> 29KViewDownload
>
>  Screen Shot 2012-01-13 at 17.44.30.png
> 33KViewDownload

[Victor Kazarinov]

Vincent,

I found new bug with list of folders. When list is bigger than height of browser, the "+" button to add new folder is appeared in wrong location. You can see this at screenshot attached to this email. To reproduce bug:

1. make list bigger than height of browser (e.g. expand "Others" folder and change/reduce height of browser window).

2. Scroll down list of folders.

3. When "Others" label with "+" button will disappear at top of screen, during scrolling, the new "+" button will appear somewhere in the list.