Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions app/internal_packages/composer/lib/attachments-area.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
import React from 'react';
import { Actions, AttachmentStore, Message } from 'mailspring-exports';
import { AttachmentItem } from 'mailspring-component-kit';
import { localized, Actions, AttachmentStore, Message } from 'mailspring-exports';
import { AttachmentItem, RetinaImg } from 'mailspring-component-kit';

export const AttachmentsArea: React.FunctionComponent<{ draft: Message }> = (props) => {
export const AttachmentsArea: React.FunctionComponent<{
draft: Message;
// Number of dragged-in threads whose .eml files are still being fetched from
// the sync engine. They aren't files on the draft yet, so they're rendered
// here as placeholders to show the drop was accepted.
attachingThreadCount?: number;
}> = (props) => {
const { files, headerMessageId } = props.draft;
const attachingThreadCount = props.attachingThreadCount || 0;

return (
<div className="attachments-area">
Expand All @@ -20,6 +27,16 @@ export const AttachmentsArea: React.FunctionComponent<{ draft: Message }> = (pro
onRemoveAttachment={() => Actions.removeAttachment(headerMessageId, file)}
/>
))}
{attachingThreadCount > 0 && (
<div className="attaching-messages">
<RetinaImg name="inline-loading-spinner.gif" mode={RetinaImg.Mode.ContentPreserve} />

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This image looks like shit, can you use the Spinner component? In a follow-up we'll clean up other remaining uses of this. The problem is it doesn't look good in dark mode and can't be inverted.

<span>
{attachingThreadCount === 1
? localized('Attaching message…')

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can we collapse this into just Attaching... and remove the attachingThreadCount and rename the prop to attaching: boolean, I think that'd be cleaner and make this more portable to future use

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think we do have to keep the state at the composer-view layer for tracking when we're done, just remove it from this component

: localized('Attaching %1$@ messages…', attachingThreadCount)}
</span>
</div>
)}
</div>
);
};
74 changes: 72 additions & 2 deletions app/internal_packages/composer/lib/composer-view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
DraftStore,
DraftEditingSession,
MessageWithEditorState,
EmlUtils,
File,
} from 'mailspring-exports';
import { webUtils } from 'electron';
Expand Down Expand Up @@ -41,6 +42,7 @@ interface ComposerViewState {
quotedTextHidden: boolean;
quotedTextPresent: boolean;
isDropping: boolean;
attachingThreadCount: number;
}
// The ComposerView is a unique React component because it (currently) is a
// singleton. Normally, the React way to do things would be to re-render the
Expand Down Expand Up @@ -78,6 +80,7 @@ export default class ComposerView extends React.Component<ComposerViewProps, Com

this.state = {
isDropping: false,
attachingThreadCount: 0,
quotedTextPresent: hasBlockquote(draft.bodyEditorState),
quotedTextHidden: hideQuotedTextByDefault(draft),
};
Expand Down Expand Up @@ -202,7 +205,7 @@ export default class ComposerView extends React.Component<ComposerViewProps, Com
</>
)}

<AttachmentsArea draft={draft} />
<AttachmentsArea draft={draft} attachingThreadCount={this.state.attachingThreadCount} />
</div>
<div className="composer-footer-region">
<InjectedComponentSet
Expand Down Expand Up @@ -278,7 +281,14 @@ export default class ComposerView extends React.Component<ComposerViewProps, Com
const hasNativeFile = event.dataTransfer.types.includes('Files');
const hasNonNativeFilePath = nonNativeFilePath !== null;

return hasNativeFile || hasNonNativeFilePath;
return hasNativeFile || hasNonNativeFilePath || this._hasThreadsForDrop(event);
};

// Threads dragged out of the thread list carry their ids in a custom MIME
// type. Note that we can only look at `types` here — the payload itself is
// not readable until the drop actually happens.
_hasThreadsForDrop = (event: React.DragEvent<HTMLDivElement>) => {
return event.dataTransfer.types.includes('mailspring-threads-data');

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

can you make mailspring-threads-data a constant and import it?

};

_nonNativeFilePathForDrop = (event: React.DragEvent<HTMLDivElement>) => {
Expand Down Expand Up @@ -313,6 +323,66 @@ export default class ComposerView extends React.Component<ComposerViewProps, Com
this._onFileReceived(uri);
event.preventDefault();
}

// Accept drops of threads from the thread list, attaching each one as a
// .eml file. dataTransfer is only valid for the duration of this handler,
// so read the payload now and hand the ids off to an async worker.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

please remove this comment it's self explanatory

if (this._hasThreadsForDrop(event)) {
this._onThreadsReceived(event.dataTransfer.getData('mailspring-threads-data'));
event.preventDefault();
}
};

_onThreadsReceived = async (json: string) => {
let threadIds: string[] = [];
try {
threadIds = JSON.parse(json).threadIds || [];
} catch (err) {
return;
}

// Dropping a thread onto a reply being written inside that same thread is
// almost always an accident, and attaching a copy of the conversation to
// itself isn't useful. Ignore it rather than surfacing an error.
threadIds = threadIds.filter((id) => id !== this.props.draft.threadId);
Comment thread
indent-staging[bot] marked this conversation as resolved.
Outdated
if (!threadIds.length) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Failing Functional · Self-thread drop guard removed

The self-drop guard that was here in the first commit (threadIds = threadIds.filter((id) => id !== this.props.draft.threadId);) was removed in aabcf6c and not replaced. For a reply draft, draft.threadId is set, so dragging that same thread onto the composer now stages its newest non-draft message and attaches the conversation to itself — the "almost always an accident" case the earlier commit deliberately suppressed, and which the PR description still lists ("Filters out drops of threads onto themselves"). If this removal was intentional, update the description; otherwise restore the filter. New-compose drafts have a null threadId, so they're unaffected either way.

return;
}

// Fetching the raw message from the sync engine is a remote round trip, so
// show a placeholder in the attachments area until the files land.
const dropCount = threadIds.length;
this.setState((state) => ({ attachingThreadCount: state.attachingThreadCount + dropCount }));

let staged: Array<{ filePath: string }> = [];
try {
staged = await EmlUtils.stageThreadsAsEml(threadIds);
} catch (err) {
AppEnv.reportError(err);
} finally {
if (this._mounted) {
this.setState((state) => ({
attachingThreadCount: state.attachingThreadCount - dropCount,
}));
}
}

if (!this._mounted) {
return;
}
for (const { filePath } of staged) {
this._onFileReceived(filePath);
}
if (staged.length < threadIds.length) {
Comment thread
indent[bot] marked this conversation as resolved.
Outdated
Comment thread
indent-staging[bot] marked this conversation as resolved.
Outdated
AppEnv.showErrorDialog(
threadIds.length === 1
? localized('Could not download the original message. Please try again.')
: localized(
'Could not download %1$@ of the original messages. Please try again.',
threadIds.length - staged.length
)
);
}
};

_onFileReceived = (filePath: string) => {
Expand Down
14 changes: 14 additions & 0 deletions app/internal_packages/composer/styles/composer.less
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,20 @@ body.platform-win32 {
}
}

// Placeholder shown while a thread dragged onto the composer is being fetched
// from the sync engine and written out as a .eml file.
.attaching-messages {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

can you align this with the real attachment items better?

Image

display: flex;
align-items: center;
padding: @spacing-half 0;
color: @text-color-subtle;
font-size: @font-size-smaller;

img {
margin-right: @spacing-half;
}
}

// Overrides for the full-window popout composer
.composer-full-window {
width: 100%;
Expand Down
27 changes: 5 additions & 22 deletions app/internal_packages/message-list/lib/message-list.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -150,29 +150,12 @@ class MessageList extends React.Component<Record<string, unknown>, MessageListSt
if (!message || !this.state.currentThread) {
return;
}
const pathModule = require('path');
const fs = require('fs');

// Use a unique subdirectory per operation so concurrent forwards don't
// race on the same file. The basename stays "Forwarded Message.eml" so
// the attachment has a clean display name.
const tempDir = pathModule.join(
require('@electron/remote').app.getPath('temp'),
`mailspring-fwd-${message.id}`
);
fs.mkdirSync(tempDir, { recursive: true });
const tempPath = pathModule.join(tempDir, 'Forwarded Message.eml');

const task = new GetMessageRFC2822Task({
messageId: message.id,
accountId: message.accountId,
filepath: tempPath,
const staged = await EmlUtils.stageMessagesAsEml([message], {
filename: 'Forwarded Message.eml',
});
Actions.queueTask(task);
await TaskQueue.waitForPerformRemote(task);

// Verify the file was actually written before creating a draft
if (!fs.existsSync(tempPath)) {
// The fetch is remote and can fail without writing anything
if (!staged.length) {
AppEnv.showErrorDialog(
localized('Could not download the original message. Please try again.')
);
Expand All @@ -191,7 +174,7 @@ class MessageList extends React.Component<Record<string, unknown>, MessageListSt
await TaskQueue.waitForPerformLocal(syncTask);

Actions.addAttachment({
filePath: tempPath,
filePath: staged[0].filePath,
headerMessageId: draft.headerMessageId,
onCreated: () => {
Actions.composePopoutDraft(draft.headerMessageId);
Expand Down
41 changes: 10 additions & 31 deletions app/internal_packages/thread-list/lib/thread-list-context-menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,30 +165,15 @@ export default class ThreadListContextMenu {
label: localized('Forward as Attachment'),
click: async () => {
const thread = this.threads[0];
const messages = await DatabaseStore.findAll<Message>(Message, { threadId: thread.id })
.order(Message.attributes.date.descending())
.limit(1);
const messages = await EmlUtils.newestExportableMessagesForThreadIds([thread.id]);
if (!messages.length) return;

const message = messages[0];
const pathModule = require('path');
const fs = require('fs');
const tempDir = pathModule.join(
require('@electron/remote').app.getPath('temp'),
`mailspring-fwd-${message.id}`
);
fs.mkdirSync(tempDir, { recursive: true });
const tempPath = pathModule.join(tempDir, 'Forwarded Message.eml');

const task = new GetMessageRFC2822Task({
messageId: message.id,
accountId: message.accountId,
filepath: tempPath,
const staged = await EmlUtils.stageMessagesAsEml([message], {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

can we write a wrapper stageMessageAsEml (singular) and do the array wrapping and unwrapping in there in the EmlUtils? It feels like this is awkward and we repeatedly reference staged[0] - i think at the very least it should be destructured right here so it's all colocated in these three lines

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

edit: i think we actually want stageThreadAsEml (signular)

filename: 'Forwarded Message.eml',
});
Actions.queueTask(task);
await TaskQueue.waitForPerformRemote(task);

if (!fs.existsSync(tempPath)) {
if (!staged.length) {
AppEnv.showErrorDialog(
localized('Could not download the original message. Please try again.')
);
Expand All @@ -207,7 +192,7 @@ export default class ThreadListContextMenu {
await TaskQueue.waitForPerformLocal(syncTask);

Actions.addAttachment({
filePath: tempPath,
filePath: staged[0].filePath,
headerMessageId: draft.headerMessageId,
onCreated: () => {
Actions.composePopoutDraft(draft.headerMessageId);
Expand Down Expand Up @@ -340,9 +325,7 @@ export default class ThreadListContextMenu {
click: async () => {
if (this.threadIds.length === 1) {
const thread = this.threads[0];
const messages = await DatabaseStore.findAll<Message>(Message, { threadId: thread.id })
.order(Message.attributes.date.descending())
.limit(1);
const messages = await EmlUtils.newestExportableMessagesForThreadIds([thread.id]);
if (!messages.length) return;

const message = messages[0];
Expand All @@ -369,15 +352,11 @@ export default class ThreadListContextMenu {
const outputDir = selected[0];
const path = require('path');

for (const thread of this.threads) {
const messages = await DatabaseStore.findAll<Message>(Message, {
threadId: thread.id,
})
.order(Message.attributes.date.descending())
.limit(1);
if (!messages.length) continue;
const messages = await EmlUtils.newestExportableMessagesForThreadIds(
this.threads.map((t) => t.id)
);

const message = messages[0];
for (const message of messages) {
const filename = EmlUtils.defaultEmlFilename(message.subject);

const task = new GetMessageRFC2822Task({
Expand Down
Loading
Loading