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

Message Forwarding #3688

Merged
merged 10 commits into from May 28, 2017

Conversation

@t3chguy
Copy link
Collaborator

t3chguy commented Apr 21, 2017

for #2597 and #3592

soft-depends on matrix-org/matrix-react-sdk#812

t3chguy added 2 commits Apr 21, 2017
Conform this file to eslint

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
t3chguy added 4 commits Apr 24, 2017
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
…forward_message

Conflicts:
	.gitignore
otherwise the server will just reject it anyway

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Apr 30, 2017

I just had a quick play (haven't looked at the code yet), but it seems pretty good. One weirdness is that the RightPanel spontaneously collapses whilst forwarding the message? Another is the UnknownDeviceDialog, which appeared when fwding into an e2e room, but seemed to then behave correctly. Are there some situations still where it gets stuck however?

@ara4n ara4n assigned t3chguy and unassigned ara4n Apr 30, 2017
@t3chguy

This comment has been minimized.

Copy link
Collaborator Author

t3chguy commented Apr 30, 2017

@ara4n I intentionally hide the right panel to make it even clearer where it wants you to interact. It restores the RightPanel when you're done only if it was visible prior to mounting
I'm hooking into the UDD in an identical way to the composer so I can't forsee any stuck state and haven't ran into one when testing

@t3chguy

This comment has been minimized.

Copy link
Collaborator Author

t3chguy commented Apr 30, 2017

as an alternative to hiding it restoring it I could break the set opacity into left/right and dim the right panel
ptal @ara4n

@t3chguy t3chguy assigned ara4n and unassigned t3chguy May 5, 2017
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented May 18, 2017

yup, i'd definitely dim the right panel rather than have it pop in & out of existence - it's why the dimming stuff exists :)

@t3chguy

This comment has been minimized.

Copy link
Collaborator Author

t3chguy commented May 18, 2017

@ara4n it wasn't possible to dim the right panel alone so wasn't sure whether that'd be something that should be possible to do or whether that was straying too far but I'll whip it up tomorrow

edit: think I'll also look into highlighting the event being forwarded, to make it clearer

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented May 18, 2017

it's not straying too far, i think it's the right UX (if going down this path). That said, i'm having another play atm to try to decide if this is actually how we want to do this... (although if it isn't, this is probably good enough for now)

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented May 18, 2017

right, i've played with it and i think this is great for now, and probably the forseeable. Only requirements are to:

  • fade out rather than hide the RightPanel
  • change the verbiage to a simple "Please select the destination room for this message" (i'd do it as a single <h1/> or whatever.
  • Add a X close button to the notification so the user can cancel out if they change their mind.

Otherwise LGTM functionalitywise. I'll check the code itself now.

onForwardClick: function() {
dis.dispatch({
action: 'forward_message',
content: this.props.mxEvent.getContent(),

This comment has been minimized.

Copy link
@ara4n

ara4n May 18, 2017

Member

hm, what about event type? what happens if you try to forward a non m.room.message? I suspect we should forbid forwarding of state events(!)

This comment has been minimized.

Copy link
@t3chguy

t3chguy May 18, 2017

Author Collaborator

if (this.props.mxEvent.getType() === 'm.room.message') {

This comment has been minimized.

Copy link
@t3chguy

t3chguy May 18, 2017

Author Collaborator

there's simply no forward button on non-m.room.messages

This comment has been minimized.

Copy link
@ara4n

ara4n May 18, 2017

Member

oh, i see we're limiting it to m.room.message. still feels a shame not to spell that out in the params passed here

This comment has been minimized.

Copy link
@t3chguy

t3chguy May 18, 2017

Author Collaborator

its currently forward_message, will be extended to forward_event, not for state events and maybe some others blacklisted and whatnot if the need is there

@@ -142,6 +151,19 @@ module.exports = React.createClass({
);
}

if (this.props.mxEvent.getType() === 'm.room.message') {

This comment has been minimized.

Copy link
@ara4n

ara4n May 18, 2017

Member

hm, i guess limiting it to m.room.message is okay for now, but this will need to change in future when arbitrary events are more comment

if (this.props.mxEvent.getType() === 'm.room.message') {
const content = this.props.mxEvent.getContent();
if (content.msgtype // truthy check msgtype
&& content.msgtype !== 'm.bad.encrypted'

This comment has been minimized.

Copy link
@ara4n

ara4n May 18, 2017

Member

this indentation feels very wrong - please can you indent the operators here?

if (foo
    && bar
    && baz)
{
    ...
}

is probably the right way to do it?

This comment has been minimized.

Copy link
@t3chguy

t3chguy May 18, 2017

Author Collaborator

since the change of the line limit I have changed this to a single line in my working dir

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented May 18, 2017

lgtm codewise too other than minor feedback

t3chguy added 2 commits May 19, 2017
button is shown, i.e easy to modify

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy

This comment has been minimized.

Copy link
Collaborator Author

t3chguy commented May 19, 2017

Added functionality of highlighting event currently being forwarded, this is fully non-destructive, overrides highlight only while the forwardingEvent is set (and so fwding aux panel is open)

Any event can be forwarded through the react-sdk portion of things, limited currently by the condition of which events the button appears for, what condition do you suggest I set there?

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented May 28, 2017

there are some slight concerns about the UX, but I don't have a better proposal right now and I think we should merge it anyway :)

@ara4n ara4n merged commit 0c140df into vector-im:develop May 28, 2017
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@@ -205,7 +206,7 @@ module.exports = React.createClass({
externalURLButton = (
<div className="mx_MessageContextMenu_field">
<a href={ this.props.mxEvent.event.content.external_url }
rel="noopener" target="_blank" onClick={ this.closeMenu }>Source URL</a>
rel="noopener" target="_blank" onClick={ this.closeMenu }>{ _t('Source URL') }</a>

This comment has been minimized.

Copy link
@t3chguy

t3chguy May 28, 2017

Author Collaborator

how'd that get there :P?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.