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.

Sign up for GitHub

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

Jump to bottom

Keep track of preview visibility on the server so it persists at page reload #1366

Merged
merged 1 commit into from
Jul 26, 2017
Merged

Keep track of preview visibility on the server so it persists at page reload #1366

merged 1 commit into from
Jul 26, 2017

Conversation

astorije
Copy link
Member

Alright, this will be my last preview-related enhancement for this release.

This is going to be especially useful because of the support for multiple previews per message + the image viewer cycling. Before this PR, collapsing NSFW previews then refreshing then cycling on image previews would display them back, oops!

One issue to keep in mind with this implementation is that running /collapse//expand will emit as many times as there are previews to collapse/expand. I don't think this is an issue that is going to have tremendous repercussions performance-wise, so I think any improvements over this can be done in a further PR (help welcome!).

@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jul 24, 2017
@astorije astorije added this to the 2.4.0 milestone Jul 24, 2017
@astorije astorije requested a review from xPaw July 24, 2017 06:05
xPaw
xPaw reviewed Jul 24, 2017
View reviewed changes
src/server.js Outdated
@@ -280,6 +280,14 @@ function init(socket, client) {
client.names(data);
}
);

socket.on("msg:preview:toggle", function(data) {
const chan = client.find(data.target).chan;
Copy link
Member

Choose a reason for hiding this comment

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

Add undefined checks for all these find

Copy link
Member Author

Choose a reason for hiding this comment

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

@xPaw, done, are you okay with how I did that?

@xPaw
Copy link
Member

xPaw commented Jul 24, 2017

So doing /collapse is going to spam the server with many events, instead of sending one in a list. Something to keep in mind.

@astorije
Copy link
Member Author

So doing /collapse is going to spam the server with many events, instead of sending one in a list. Something to keep in mind.

Right, mentioned that in the PR description when I opened it.
I don't think it's a blocker for this PR, but regardless, do you think it's going to be a performance issue since it's going through a websocket, as opposed to plain HTTP request? After all, the same information is being passed to the server, just that there is less socket overhead with sending a list. Note that I'm ignorant of the implementation details of websockets and that might make this question silly :)

xPaw
xPaw reviewed Jul 25, 2017
View reviewed changes
src/server.js Outdated

socket.on("msg:preview:toggle", function(data) {
const networkAndChan = client.find(data.target);
if (networkAndChan) {
Copy link
Member

Choose a reason for hiding this comment

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

You could just return in each if instead of going deeper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I honestly prefer the nested version, shorter and makes more sense to me, but whatever.

Adding do not merge label so I remember to squash before merging.

@xPaw
Copy link
Member

xPaw commented Jul 25, 2017 •
edited
Loading

Maybe for collapse command we can do an optimization trick where it sends msg:preview:collapse just for the channel, and msg:preview:toggle is not sent for each click (not sure how you're going to filter that one out), and then the server just loops messages and sets shown to false on all of them (and backwards for expand). You probably can include first visible message id in it (same how more works) to not toggle stuff in buffer that you can't see.

@astorije
Copy link
Member Author

Agreed we can probably benefit from optimization later, but I won't have the cycles to do that right now. I'll let someone else chime in in a further PR, though I don't believe this will be an issue anyway.

@astorije
Copy link
Member Author

Otherwise, I believe this PR is up for review :)

@astorije astorije mentioned this pull request Jul 26, 2017
Closed
@astorije astorije force-pushed the astorije/persist-preview-toggle branch from 68be238 to 1572892 Compare July 26, 2017 22:16
@astorije astorije merged commit 8aa89d7 into master Jul 26, 2017
@astorije astorije deleted the astorije/persist-preview-toggle branch July 26, 2017 22:26
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@astorije
Merge pull request thelounge#1366 from thelounge/astorije/persist-pre…
1191063 
…view-toggle

Keep track of preview visibility on the server so it persists at page reload
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xPaw xPaw xPaw approved these changes

Assignees
No one assigned
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@astorije @xPaw