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

Ways to edit attachments #182

Closed
romanofski opened this issue Jun 29, 2018 · 9 comments
Closed

Ways to edit attachments #182

romanofski opened this issue Jun 29, 2018 · 9 comments
Assignees
Labels
Milestone

Comments

@romanofski
Copy link
Member

We need a way to view/edit attachments.

My initial thoughts were to add a parser for a mailcap file, but in 2018 perhaps there are better ways.

@romanofski
Copy link
Member Author

After implementing #10, I think what would be useful to add in the configuration is a:

Map ContentType (ProcessConfig ()()())

Meaning, if we can lookup the ProcessConfig type (which supports IsString) by the given ContentType use it as a command, otherwise prompt the user.

Perhaps a Plugin could be later utilized which could for example parse an existing mailcap file and provide the Map ...

@romanofski
Copy link
Member Author

I looked into this. The tricky bit could be ProcessConfig and using the above type, because we're getting into trouble with NFData:

src/Types.hs:348:24-29: error:
    • No instance for (NFData (ProcessConfig () () ()))
        arising from the 'deriving' clause of a data type declaration
      Possible fix:
        use a standalone 'deriving instance' declaration,
          so you can specify the instance context yourself
    • When deriving the instance for (NFData MailViewSettings)
    |
348 |     deriving (Generic, NFData)
    |                        ^^^^^^

It seems ProcessConfig is only exported as a type from the API which will make it harder for us to declare the NFData instance.

@frasertweedale
Copy link
Member

@romanofski I think we want a richer type anyway - one that we can easily convert into a ProcessConfig - that allows more fine-grained control over where the filename appears, and supports pipes (perhaps). For now I would say make the simplest data type that expresses what we currently support, create an NFData instance for it, and put that in the config. Then we convert that to a ProcessConfig just in time to actually use it.

Alternatively, just make the NFData instance of the config record type that immediately includes the ProcessConfig values skip the ProcessConfig values. It's not lawful, and not ideal, but there is precedent: see instance NFData (Keybinding v ctx) in Types.hs (there is no NFData instance for Vty.Event so I skip it.)

@romanofski
Copy link
Member Author

I tried the latter, but that means I can pattern match on the ProcessConfig constructor, yes? Since the constructor is not exported I think I need to pick the first idea.

@frasertweedale
Copy link
Member

@romanofski write the NFData instance for the thing that contains the ProcessConfig, and just don't force the ProcessConfig value. There's no need to pattern match.

Push your WIP and let me help if you are unsure how to continue.

@romanofski
Copy link
Member Author

Cheers! I'll try it out. I'll finish #263 off first since it's mostly mechanical work ...

romanofski added a commit that referenced this issue Mar 30, 2019
This adds a proof of concept mailcap declaration. If we can lookup a command,
use it as a preference otherwise show an input editor. Keep the possibility to
open the attachment with a different command by binding the input editor to a
different keystroke tho. This patch would also fix #174 since it is now possible
to automatically invoke a program to see HTML output.

Fixes #182 and
      #174
romanofski added a commit that referenced this issue Mar 30, 2019
This adds a proof of concept mailcap declaration. If we can lookup a command,
use it as a preference otherwise show an input editor. Keep the possibility to
open the attachment with a different command by binding the input editor to a
different keystroke tho. This patch would also fix #174 since it is now possible
to automatically invoke a program to see HTML output.

Fixes #182 and
      #174
romanofski added a commit that referenced this issue Mar 30, 2019
This adds a proof of concept mailcap declaration.

If the lookup for a content type is successful, we use the mapped command,
otherwise show an input editor. Keep the possibility to open the attachment with
a different command by binding the previous open action to a different
keystroke.

A sample mailcap is used and adds a mapping from text/html, application/pdf to
xdg-open. This patch would also fix #174 since it is now possible to
automatically invoke a program to view an HTML attachment.

Fixes #182 and
      #174
romanofski added a commit that referenced this issue Mar 30, 2019
This adds a proof of concept mailcap declaration.

If the lookup for a content type is successful, we use the mapped command,
otherwise show an input editor. Keep the possibility to open the attachment with
a different command by binding the previous open action to a different
keystroke.

A sample mailcap is used and adds a mapping from text/html, application/pdf to
xdg-open. This patch would also fix #174 since it is now possible to
automatically invoke a program to view an HTML attachment.

Fixes #182 and
      #174
@romanofski romanofski self-assigned this Mar 30, 2019
@romanofski romanofski modified the milestones: Future Feature, 1.0 Mar 30, 2019
romanofski added a commit that referenced this issue Mar 31, 2019
If the lookup for a content type is successful, we use the mapped command,
otherwise show an input editor. Keep the possibility to open the attachment with
a different command by binding the previous open action to a different
keystroke.

A sample mailcap is used and adds a mapping from text/html to elinks and a catch
all to xdg-open. This patch would also fix #174 since it is now possible to
automatically invoke a program to view an HTML attachment.

Fixes #182 and
      #174

erasf
romanofski added a commit that referenced this issue Mar 31, 2019
If the lookup for a content type is successful, we use the mapped command,
otherwise show an input editor. Keep the possibility to open the attachment with
a different command by binding the previous open action to a different
keystroke.

A sample mailcap is used and adds a mapping from text/html to elinks and a catch
all to xdg-open. This patch would also fix #174 since it is now possible to
automatically invoke a program to view an HTML attachment.

Fixes #182 and
      #174

erasf
@romanofski
Copy link
Member Author

Will have to open this, since I filed this when I was hacking on listing attachments. I stumbled over a comment:

-- in src/UI/Actions.hs
        Nothing -> pure $ setError (GenericError "No file selected to edit") s
        Just (_, m) -> case preview (headers . contentDisposition . dispositionType) m of
          (Just Inline) -> invokeEditor' s
          _ -> pure $ setError (GenericError "Not implemented. See #182") s

Stumbling over this FIXME string actually reminded me of what #182 originally was intended when I filed it: The ability that the author of an e-mail can edit attachments on a newly composed email. We currently support only text/plain editing of the inline body, but this issue was intended that any attachment can be edited, saved and updated on composed mail.

@romanofski romanofski reopened this May 31, 2019
@romanofski
Copy link
Member Author

I'm thinking of adding an additional mailcap setting for editing attachments, e.g.

data MailViewSettings = MailViewSettings
    {  -- [...]
    , _mvMailcap :: [(ContentType -> Bool, MailcapHandler)]
    }

to

data MailViewSettings = MailViewSettings
    {  -- [...]
    , _mvOpenMailcap :: [(ContentType -> Bool, MailcapHandler)]
    , _mvEditMailcap :: [(ContentType -> Bool, MailcapHandler)]
    }

and use mvEditMailcap for editing attachments when composing mails, while mvOpenMailcap is used to open attachments from received mails. Both can default to use xdg-open ...

@romanofski
Copy link
Member Author

In the discussion for #318 we concluded to postpone this until we have a tree widget. The card to close this off is #322

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants