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.

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

Add a method to list subfolders of a MailFolder recursively #293

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

Moerfi666
Copy link
Contributor

Also as discussed on Matrix with @seijikun,
here the PR that adds an implementation for recursive folder listing.

This takes an optional parent folder, and returns all folders and subfolders in the hierarchy as flattened list.
So a hierarchy:

- myfolder1
  - myfolder12
  - myfolder13
  - myfolder14
    - myfolder141

becomes this flattened sequential list:

- myfolder1
- myfolder1 / myfolder12
- myfolder1 / myfolder13
- myfolder1 / myfolder14
- myfolder1 / myfolder14 / myfolder141

Normally, IMAP supports this operation directly (O(1)), but JavaMail does not seem to provide this functionality, forcing one to traverse the hierarchy manually.


private def listRecursive(
folder: Folder,
folderList: List[Folder] = Nil
Copy link
Owner

Choose a reason for hiding this comment

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

Hm… it seems the folder is never added to the result? So "INBOX" doesn't belong to the result - or the parent folder? Then folderList is unused, the method is not called with other than Nil.

It would be nice, if it would be tail-recursive, wdyt? Probably not so important in practice, but I tend to prefer it in general as it is usually not harder to write/read, imho, and so we get stack safety from scala.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right about the folderList parameter, I was a bit tired writing this, sorry.

Not including the parent folder is intended, maybe renaming to ListSubFolders would be more appropriate?

How would someone transform this into a tail-recursive method? Maybe I just can't wrap my head around it, but it seems to be hard because it's a tree like structure - I'm open for ideas!

Copy link
Owner

@eikek eikek Dec 17, 2021

Choose a reason for hiding this comment

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

Not including the parent folder is intended, maybe renaming to ListSubFolders would be more appropriate?

Ah ok - I looked at the example in the description, it seems to be included there. Maybe we can return a NonEmptyList with the input folder as first element? The user can then decide whether to use it or not. I thought it may be convenient when returning the INBOX when giving a None as input.

Regarding the recursive thing. I thought maybe something like this could work:

@annotation.tailrec
def flatten(folder: Vector[Folder], result: Vector[Folder] = Vector.empty): Vector[Folder] = 
  if (folder.isEmpty) result
  else flatten(folder.flatMap(_.list().tovector), folder ++ result)

// flatten(Vector(parent)) 

(modulo all the syntax errors) But I may just miss something here! can totally relate to the tiredness:) then just leave it - it's not important anyways.

@Moerfi666
Copy link
Contributor Author

Moerfi666 commented Jan 7, 2022

We've converted our method to be tail recursive now.
One problem with having the requested folder as part of the result is when calling listRecursive(None) (so, essentially getting a list of top-most folders), because
it would then add the IMAP drive's root folder into the list.
But that folder is transparent and has no path, which is represented as a MailFolder instance with an empty NonEmptyList for path.

@eikek
Copy link
Owner

eikek commented Jan 7, 2022

We've converted our method to be tail recursive now. One problem with having the requested folder as part of the result is when calling listRecursive(None) (so, essentially getting a list of top-most folders), because it would then add the IMAP drive's root folder into the list. But that folder is transparent and has no path, which is represented as a MailFolder instance with an empty NonEmptyList for path.

Ah yes, that is true! I would be ok with not adding it in the None case. But otoh this feels quite surprising, then I'm also for removing the input folder from the results. My thoughts for including it first, were more from a ergonomic point of view. I think I wouldn't trade it for surprising behaviour 🤔

@eikek
Copy link
Owner

eikek commented Jan 14, 2022

Hey @Moerfi666 if you want to rather remove the input folder from the results, I'm totally fine with that. I'm also fine with current state, let me know what you think/want to do. No need to hurry, just wanting to let you know …

@Moerfi666
Copy link
Contributor Author

Hey! Sorry for the long delay.

Conceptually, we thought about the method more as a way to get the subfolders within a given path.
So we would prefer to stay with the current version.
(Also to avoid the IMAP-induced special cases the other option would require.)

@eikek eikek merged commit c8c1a3e into eikek:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants