848400 - [E-Mail] Support attaching any file available on the SD card(s)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P3)

Description

[David Palomino [:dpalomino]]

As a user I would like to be able to attach any file available in the SD Card when composing an e-mail.

RATIONALE: In case a user has downloaded a content he cannot see in the device he is likely to be looking for an alternative way to use it. But it is not possible to send it through e-mail either.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

This requires a 'pick' activity that can list all of the files on the SD card.  This then requires the more minor change to the e-mail app to structure its 'pick' activity request so that said activity is eligible.  I belive UX has an idea for some type of "document manager", but it's unlikely to be a minor undertaking.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

pdol, please mark these features that are to be in 1.1 as leo?

FYI: needed to be +'d for 1.1 work since its a feature/user story

Comment 3

[Peter Dolanjski [:pdol]]

Naoki, sorry for the confusion.  I added the "feature" keyword to anything that is a new feature request (as opposed to a defect), but in this case we definitely aren't considering it for leo, so I didn't nom it.  We may consider it in the future, at which point we'll nom it for whatever that release is.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

So there's two things that need to happen for this:
1) Email needs to be able to support anything via pick/share activities.
2) Other apps need to exist to be able to expose the things on the SD card.  I think the download manager may have largely addressed this already.

To address the first case I've created a new dev.gaia thread to discuss it.  Please see:
https://groups.google.com/d/msg/mozilla.dev.gaia/Dp1fbH9-Uh4/LCEmY_1pyOkJ

Comment 6

[Marcelino Veiga Tuimil [:sonmarce]]

We were discussing internally about a way to expose to E-Mail app all contents downloaded by Download Manager. What we propose is to implement a hidden app for browsing all downloaded contents and providing a pick activity with a special type like application/file. E-Mail app would only need to add this application/file type to its expected list of types when calling pick activity.

What do you think?

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

The core idea sounds good.  But maybe we should use application/octet-stream instead of application/file?  Specifically, I've never seen application/file used anywhere, whereas application/octet-stream does seem to be the convention for unknown file types.  On https://groups.google.com/d/msg/mozilla.dev.gaia/Dp1fbH9-Uh4/LCEmY_1pyOkJ I also proposed application/* which maybe could also work if you want to avoid there being an explicit mime type?  (We already do wildcards for image/* and video/*).

Comment 8

[Marcelino Veiga Tuimil [:sonmarce]]

No problem with content type, the one you prefer would be good for us :-) So, let´s ask for feedback to Product people of Systems FE for the hidden app, as it would be part of download manager work.

Peter, what do you think about having a hidden app to expose downloaded contents to other apps? (see comment 6)

Comment 9

[Cristian Rodriguez (:crdlc)]

I've implemented a prototype to show the files downloaded:

Demo: http://youtu.be/f0kyZIgUDpY

Code: https://github.com/mozilla-b2g/gaia/pull/24002

Basically it implements a new activity:

new MozActivity({
  name: 'pick', 
  data: {
    type: 'application/*'
  }
});

that shows the downloaded files and when users click on them the caller receives a object like this

{
  type: 'the mime type of the file'
  blob: 'the binary content',
  filename: 'name of the file'
}

Comment 10

[Rob MacDonald [:robmac]]

Hi everyone...

Sorry but the requirement here is not clear to me. The original user story is "attach any file available in the SD Card when composing an e-mail" but files that are associated with an existing app are already covered using the attachment picker. Music, Gallery, etc., should already be scanning and picking up content from all drives.

If the problem we're trying to solve here one of managing files that are not associated with any apps or attaching files from an SD card, I think we're talking about a much broader issue. We get into issues of deleting, sharing and moving the files... which gets us more into document manager territory.

I'd propose that we'd defer this patch until further analysis can take place. Although it hasn't been scheduled, this is an item that is in the current backlog and is being considered as part of our current planning process.

-  Rob

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Rob MacDonald [:robmac] from comment #10)
> If the problem we're trying to solve here one of managing files that are not
> associated with any apps or attaching files from an SD card, I think we're
> talking about a much broader issue. We get into issues of deleting, sharing
> and moving the files... which gets us more into document manager territory.

I understand the goal of what :sonmarce and :crdlc have suggested and implemented is to expose files known to the download manager to the email app, not arbitrary contents of the SD card.  Obviously this is only partial progress towards the current bug subject (attach anything on an SD card).

I guess the question is what is the expected plan for v2.2.  Because email is fairly strongly committed to addressing "forwarding with attachments" (bug 912002) for v2.2 and it extensively overlaps with properly addressing saving any/all attachments to the download manager (bug 825318), either the proposed-here "download manager pick activity" or the full-featured "document manager with pick support" should ideally be implemented for download/pick-share symmetry.

However, the "share" half of pick-share is already within reach since the download manager already supports "share" functionality.  The main problem is that the email app's activity is filtered to the MIME types that the email app knows how to handle because it would be more ridiculous if you couldn't download/view attachments you sent from than email app than the current status quo.


I think the ideal course of action is to have the discussion in dev-gaia with the various systems front-end/download manager people (engineers and product) about how they see this going for v2.2.  The email-app specific aspect of this is really just the MIME type and email being able to be able to actually view attachments and hand them off to the download or document manager.  Based on various factors I don't see email being able to do that until November or so, unless non-MoCo-email-app resources are brought to bear on bug 825318 earlier to advance the in-progress fix there.

Comment 12

[Marcelino Veiga Tuimil [:sonmarce]]

Having a document/file manager to browse/expose any content from SD card would be the ideal solution, but it is not possible in the short term as there is no Product/UX plan yet. This is the reason why we are proposing to start with downloaded contents, we have download manager being able to fully handle any kind of downloaded files, just remaining to expose those contents to other apps like E-mail app.

In this case, we can take advantage of current implementation of download manager, with a fully consistent UX spec, to add value to end users. It would be also possible to implement it in the short term. Indeed, we are finishing UX spec, and also have an initial implementation waiting for being adapted when UX spec is finished.

I would suggest we continue working in the UX spec, then attach it to this bug for you to review it, and finally adapt implementation to final UX spec. What do you think?

Comment 13

[Pau Masiá [:Pau]]

Attachment: UX Spec. Email Attaching downloaded files

Here I attach the UX spec proposal for this scenario.

Rob, could you review everything is ok, please?

Thanks!

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

This is still really not the right Bugzilla Component for this.  Any patch is like 2 lines of code in email and everything else is in Systems Front-End somewhere.  No one in email has review authority over such code.  Please move the discussion to an appropriate venue, such as one of the Gaia::System::* components or dev-gaia.

Comment 15

[Rob MacDonald [:robmac]]

Hello...

I really think this is something that needs to be considered as part of the Systems FE backlog as part of our explorations for managing storage and unassociated file types. To implement this solution now would be premature as it could set the wrong expectations for users and would likely be something that we'd need to back out of in future releases.

Flagging Peter as owner of the SFE backlog.

Best regards,
Rob

Comment 16

[Marcelino Veiga Tuimil [:sonmarce]]

Are there plans for having a document/file manager in 2.2? If not, I think this approach of exposing downloaded contents is better than nothing.

Comment 17

[Rob MacDonald [:robmac]]

(In reply to Marcelino Veiga Tuimil [:sonmarce] from comment #16)
> Are there plans for having a document/file manager in 2.2? If not, I think
> this approach of exposing downloaded contents is better than nothing.

Hi Marce and Pau...

The timing for a document manager is still an open issue at this point. I understand the issue with unassociated files, though, so we can keep you posted. If we're not able to address it for 2.2, a patch such as this could be used to customize select devices until we're able to implement a broader solution. 

Feel free to email me if you want to meet to discuss this further. Otherwise I'll keep you posted on 2.2.

Best regards,
Rob

Comment 18

[Marcelino Veiga Tuimil [:sonmarce]]

Thanks for keeping us informed about plans for 2.2. I would suggest to move forward with this solution to expose downloaded content, meanwhile we do not know plan for 2.2. It is an easy patch and there would be no problem to back it out if we finally have a better plan for 2.2. It could also be disabled by default (just not installing the hidden app), leaving the OEM decide about including it or not in their commercial builds.

Comment 19

[Peter Dolanjski [:pdol]]

Sorry for the delay Marce.  We discussed this internally.  Please go ahead and land it when ready, but if we can have it disabled by default for now (perhaps through a developer setting) that would be ideal so that we can have people play with it and see how they like it.

Does that work for you?

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Peter Dolanjski [:pdol] from comment #19)
> Sorry for the delay Marce.  We discussed this internally.  Please go ahead
> and land it when ready, but if we can have it disabled by default for now
> (perhaps through a developer setting) that would be ideal so that we can
> have people play with it and see how they like it.

Whoa whoa whoa, developer setting is a bad idea.  What you're asking for is an app that's sometimes there and sometimes isn't, web-activity-wise.  This should not be accomplished via yet another developer setting.  If you want people to be able to experiment with it but not commit to having it in-tree, then it should just be an app that gets side-loaded via the WebIDE/App manager and not live in the tree at all.  See :jrburke's https://github.com/jrburke/gaia-dev-zip for an idea of how to accomplish that.

Rationale: "Do both" is oft times a cop-out, and it's frequently a cop-out that complicates the lives of developers.  My specific concern with this would be that I already have a hard enough time getting people to provide basic, intuitive information and useful STR steps in email bug reports; the state of a random developer setting is likely to never be provided until after I've wasted a lot of time.

Note: An engineering-build-only app is a different option, but I personally try and avoid use engineering builds anymore because of their history of completely filling up the device's partitions.  It also seems inadvisable because people would be confused by features appearing/disappearing apparently at random as they switch between builds or trigger reset-gaia with different settings/etc.

Comment 21

[Marcelino Veiga Tuimil [:sonmarce]]

No problem to disable it by default. As it will be a hidden application, we can create it in apps directory of GitHub as usual, so it will be included in engineering builds, but not in production builds unless we add it in configuration file during build time.

Comment 22

[Cristian Rodriguez (:crdlc)]

Attachment: [CLOSED] Github pull request

First round

Comment 23

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8496762 [details]
[CLOSED] Github pull request

Pretty amazing work!

Just have a couple of questions, despite of the comments I left in github:

    Are we going to show a message when there are not downloads available?
    When the list is empty I can scroll down the empty screen.

Also found an error if you just have 1 download, not an array:
E/GeckoConsole( 3026): [JavaScript Error: "TypeError: event.target.result.reverse is not a function" {file: "app://download.gaiamobile.org/js/pick.js" line: 57}]

Should be pretty quick to solve.

Last thing, we could have a listener while we are on the download picker to check for completed downloads meanwhile we are displaying the pick. But, to be honest, I would prefer to land this as it is, and in a follow up add that.

Great job Cristian!

Comment 24

[Peter Dolanjski [:pdol]]

(In reply to Andrew Sutherland [:asuth] from comment #20)
> Whoa whoa whoa, developer setting is a bad idea.  What you're asking for is
> an app that's sometimes there and sometimes isn't, web-activity-wise.  This
> should not be accomplished via yet another developer setting.  If you want
> people to be able to experiment with it but not commit to having it in-tree,
> then it should just be an app that gets side-loaded via the WebIDE/App
> manager and not live in the tree at all.  See :jrburke's
> https://github.com/jrburke/gaia-dev-zip for an idea of how to accomplish
> that.

Good call.  I forgot that this was a specific app.

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8496762 [details]
[CLOSED] Github pull request

r=asuth for the one-line change to the email app's compose.js.  I lack review authority for anything else and have not looked at anything else.  Feel free to carry this r+ forward as long as this part of the patch does not change.

Comment 26

[Cristian Rodriguez (:crdlc)]

Hi Fran, inline

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #23)
> Comment on attachment 8496762 [details]
> Github pull request
> 
> Pretty amazing work!
> 
> Just have a couple of questions, despite of the comments I left in github:
> 
>     Are we going to show a message when there are not downloads available?

I did it how the download manager does in settings. I can review my implementation just in case this is not working properly

>     When the list is empty I can scroll down the empty screen.
> 
> Also found an error if you just have 1 download, not an array:
> E/GeckoConsole( 3026): [JavaScript Error: "TypeError:
> event.target.result.reverse is not a function" {file:
> "app://download.gaiamobile.org/js/pick.js" line: 57}]

Thanks, fixing of course

> 
> Should be pretty quick to solve.
> 
> Last thing, we could have a listener while we are on the download picker to
> check for completed downloads meanwhile we are displaying the pick. But, to
> be honest, I would prefer to land this as it is, and in a follow up add that.

It seems pretty easy to do, good catch

> 
> Great job Cristian!

Comment 27

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8496762 [details]
[CLOSED] Github pull request

Hi Fran, when you have a chance please review again, I've already addressed all comments but a follow-up for "update the list with new downloads while the activity is opened" because of it is a corner case thought. Thanks a lot for your time mate

Comment 28

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

I have to open another pull request because the other one was auto-closed

Comment 29

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8496762 [details]
[CLOSED] Github pull request

I want to keep this patch here in order to have the Github comments as reference for the reviewer

Comment 30

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8498794 [details]
Github pull request

Impressive job as always, thanks for adding the suggestions.

Comment 31

[Francisco Jordano [:arcturus] [:francisco]]

Landed:

https://github.com/mozilla-b2g/gaia/commit/deb00366e771a400124bc75e49d2ca9f9d3c79a9

Comment 32

[Rob MacDonald [:robmac]]

Clearing earlier flag as question was answered by Peter and Andrew. Thanks!

Comment 33

[Marcelino Veiga Tuimil [:sonmarce]]

Opened follow up 1113152 to enable it by default in production builds