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/event pinning #5142

Merged
merged 16 commits into from Oct 15, 2017

Conversation

@turt2live
Copy link
Member

turt2live commented Sep 29, 2017

Many users have requested the ability to pin messages in a room, either for pinning the rules of the room or having more context than a topic can provide. This PR adds basic message pinning and prepares for the future of pinning threads.

This addresses #1858

Here's what it looks like (when opened):
image

  • Pin/unpin messages from context menu
  • Only support m.room.message events in Riot
  • Button for the top of the room to show/hide pinned messages

Required partner PRs

turt2live added 12 commits Sep 28, 2017
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
@turt2live turt2live referenced this pull request Sep 29, 2017
Copy link
Member

dbkr left a comment

Most of these comments are little things. The main concern is whether pinned events will fail to load for new people joining the room.

Also the format they're being stored in could lead to multiple writes clobbering each other as it's one state event with a list, but the alternative would gradually build up state since we can't delete state events, so maybe this is preferable.

@ara4n wdyt?

@@ -122,6 +133,28 @@ module.exports = React.createClass({
this.closeMenu();
},

onPinClick: function() {
MatrixClientPeg.get().getStateEvent(this.props.mxEvent.getRoomId(), 'm.room.pinned_events', '')
.then(null, e => {

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

catch?


'use strict';

var React = require('react');

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

These should really be imports in new code

limitations under the License.
*/

'use strict';

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

We shouldn't really be writing this in new files

This comment has been minimized.

Copy link
@turt2live

turt2live Sep 29, 2017

Author Member

copy/paste fails (as with most of the comments). will fix.

const PinnedEventTile = React.createClass({
displayName: 'PinnedEventTile',
propTypes: {
mxRoom: React.PropTypes.object.isRequired,

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

Ideally these would use the prop-types module in new code


let unpinButton = null;
if (this._canUnpin()) {
unpinButton = <img src="img/cancel-red.svg" className="mx_PinnedEventTile_unpinButton" width="8" height="8"

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

This should probably be an AccessibleButton?

import { _t } from "matrix-react-sdk/lib/languageHandler";
import { EventTimeline } from "matrix-js-sdk";

const PinnedEventTile = React.createClass({

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

Not a huge deal but this is probably chunky enough to warrant its own file

const cli = MatrixClientPeg.get();

pinnedEvents.getContent().pinned.map(eventId => {
promises.push(cli.getEventTimeline(this.props.room.getUnfilteredTimelineSet(), eventId, 0).then(timeline => {

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

Hmm, I don't think this will necessarily find the event as you'll only get whatever has been loaded into the unfiltered timeline set, ie. someone newly entering a room won't be able to load a pinned event posted longer ago than the brief bit of scrollback their client loads, I think.

This comment has been minimized.

Copy link
@turt2live

turt2live Sep 29, 2017

Author Member

Watching the network pane, it does a request to the context API. It does try to hit the cache first though.

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

Oh I see - I didn't realise that hit the context API! That sounds like exactly what it should be doing then.


_getPinnedTiles: function() {
if (this.state.pinned.length == 0) {
return <div>{ _t("No pinned messages.") }</div>;

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

Would we ever show this? Surely if there are no pinned messages, you'd just show nothing at the top?

This comment has been minimized.

Copy link
@turt2live

turt2live Sep 29, 2017

Author Member

The pinned messages pane is a drawer that is dismissed by default. This is more to communicate to people that there's nothing to see when they pop it open.

"Unpin Message": "Unpin Message",
"Jump to message": "Jump to message",
"No pinned messages.": "No pinned messages.",
"Loading...": "Loading...",

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

Ftr these will just fall back to UK English if they're not there so adding them here doesn't actually do anything

This comment has been minimized.

Copy link
@turt2live

turt2live Sep 29, 2017

Author Member

The only reason I include these is to make weblate not complain about en-us being <100%.

@@ -0,0 +1,171 @@
/*

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 29, 2017

Member

We do do it elsewhere at the moment, but generally putting components in riot-web that are referenced from react-sdk is a big no-no since it worsens react-sdk's dependency on riot-web.

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Sep 29, 2017

For whatever it's worth, the following semantics are applied based on the event spec:

  • If a pinned message is redacted, pretend it isn't pinned
  • If a pinned event is not a message, pretend it isn't pinned
  • If an event is not accessible, pretend it isn't pinned
    • Eg: Pinned event is before the user's join, and they can't see past history
    • Eg: Pinned event is not in the room
    • Eg: Pinned event does not exist
turt2live added 2 commits Sep 29, 2017
Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Sep 29, 2017

@dbkr Both sides updated. Thanks for the feedback!

@t3chguy t3chguy referenced this pull request Oct 3, 2017
3 of 3 tasks complete
@dbkr
dbkr approved these changes Oct 11, 2017
Copy link
Member

dbkr left a comment

OK by me - letting @ara4n take a look as it's a reasonably chunky feature

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Oct 14, 2017

okay, i'm a terrible terrible person and had completely missed/forgotten this PR whilst submerged in funding crap over the last few weeks; sorry @turt2live! @dbkr/@turt2live please stab me in the eye (via Matrix or IRL) if this happens again...

In other news, WOO!!!! This is awesome! We desperately need the feature, and this looks like a great implementation of it (modulo perhaps a little bit of CSS on my side).

My only thoughts are:

  • Please can we shove it behind a labs flag at first, so we can play with it slightly more gently in the wild?
  • While the m.room.pinned_events state event is a simple way to do this, it's worth noting that pinning could be seen as a special-case form of message reactions: the act of upvoting/downvoting etc a message is quite similar to saying "i've reacted to this message with a 'pin' reaction". So we might want to converge the two in future. Until we have reactions though I think this is a great solution.
  • We'll need to make sure it's on the radar of the mobile riot clients (which will be an interesting UX challenge, as we don't want to constantly suck up screen real estate there)

This is fantastic! :D :D :D

@ara4n ara4n assigned turt2live and unassigned ara4n Oct 14, 2017
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Oct 14, 2017

I have a feeling that using reactions for pinning might be a bit harder, although it might work out in the end (assuming there's some way to say "give me all the events that have an m.pinned reaction" with minimal performance loss).

As for the labs setting: I meant to do that, and seemingly forgot. I'll figure out how the new system works and get this PR up to speed.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Oct 14, 2017

the new system should be simpler (although I haven't touched it myself yet). And yup, using reactions would be contingent on being able to rapidly filter events in a room based on a tag, effectively. But saying "hey, give me all the events i've starred" or "give me all the pinned events" or "give me all the events tagged spam" all feel fairly reasonable things to optimise for in the long term.

Signed-off-by: Travis Ralston <travpc@gmail.com>
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Oct 14, 2017

@ara4n it's now behind a labs setting.

It won't prevent the timeline text if someone in the room pins something, but it'll prevent the happy paths for people to pin messages (context menu and the list itself).

The build error is due to matrix-org/matrix-react-sdk#1483

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Oct 14, 2017

hm. how hard is it to hide the pinned messages themselves behind the labs flag? as i'm most worried that the pinned messages may surprise/break innocent users (especially until CSS is tweaked a bit)...

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Oct 14, 2017

The pinned messages are behind a drawer that requires a manual click, like in Discord. I've put the button behind the labs flag, so the drawer can't even open without some Hard Work.

Personally I'm not a fan of pinned messages automatically showing, which is why the PR intentionally puts it behind a drawer.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Oct 15, 2017

okay. i can see a real use for pinned messages being used as a topic analog, though, but let's have a play! thanks :D

@ara4n ara4n merged commit f143315 into vector-im:develop Oct 15, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.