Completely refactor and fix timeslider by s1341 · Pull Request #2040 · ether/etherpad

s1341 · 2013-12-17T15:47:31Z

No description provided.

This commit introduces a new TimesliderClient object which will eventually handle all communication with the server in a sane fashion. At the moment, minimal work has been done to wire the existing code into this object. The current 'on("MESSAGE_TYPE")' model will change so that the timeslider object is actually the one managing all the information, instead of delegating it all out to callbacks. This current version still works just as the stock version did.

The slider handle and stars render correctly. Not yet wired up to real events.

without this, the timeslider and sliderui classes don't work

Working slider class with mouse handling (click, drag) and events (change, slide). Lots of console logs still in the code. Got rid of some of the legacy code.

Also, the steppers actually work.

findPath works. transition requests changesets from server. processing of changesets works correctly. Only need to manage the async-ifying of transition, which is currently written as a while loop, (with a short-circuit break for debugging).

There are still some weird kinks in the traversal algorithm. For example, trying to go from 9 -> 19 fails very oddly.

There are still some bugs in the traversal algorithm, as well as bugs being triggered in the server. Also, for some reason it looks like there is a problem with the attribute pool not containing certain elements.

Working: - location.hash - click on slider - stepper buttons - UI update (except authors) Not working: - playback - bugs in transition code

Handle upstream new revisions and added users/authors.

Added HUGE (1000 revs) bucket. There are still some libchangeset errors which need fixing.

s1341 · 2013-12-22T17:46:04Z

This is more or less ready. There are two outstanding issues with the changesets themselves, one of which must be resolved before we can move forward, as it leaves the pad in an unknown state:

Uncaught Error: Failed assertion: Can't have op.lines of 2 in attribution lines

I'd be grateful for some additional testing.

JohnMcLear · 2013-12-22T21:41:52Z

We have frontend automated tests for this already: peep the timeslider files in https://github.com/ether/etherpad-lite/tree/develop/tests/frontend/specs

Hope that helps :)

marcelklehr · 2013-12-30T12:20:10Z

author colors are only visible as long as you don't change the revision. Newly applied changesets don't add author colors, it seems.

marcelklehr · 2013-12-30T12:20:36Z

And I get a lot of these, (especially when going back to rev#0)

TypeError: newdivs[0] is undefined

(1033 out of range 653)

marcelklehr · 2013-12-30T13:54:07Z

(regarding that occupying char (TM)):
Running the following in the browser console (after having loaded HEAD-1 manually), always gives an Error: Failed assertion: mismatched apply: 330 / 329

var changeset = require('ep_etherpad-lite/static/js/changeset.js')
changeset.applyToAText(
  _this.revisionCache.head_revision.previous.small.value
, _this.clientVars.collab_client_vars.initialAttributedText
)

The error is thrown at this line: exports.assert(str.length == unpacked.oldLen, "mismatched apply: ", str.length, " / ", unpacked.oldLen);, so that means the passed string is one char longer than expected.

Considering that the changesets received from the server are invalid, the timeslider shouldn't work at all. The timeslider code prolly uses some line based apply method, that gives its best to cover up such mistakes, so that's why it still works somehow.

I suspect there's one \n at the end of initialattributedtext.text that doesn't belong there (there are two at the end)... But then that's how it's stored in the database :/

s1341 · 2013-12-30T14:07:51Z

Thanks for all the feedback. I'll probably only be able to do some more work on this tomorrow. The timeslider does do line-based changeset stuff.

marcelklehr · 2013-12-30T16:38:11Z

Just wrote some assertions on the server, and it seems the server-side code is broken. -- ahem.. maybe not, I dunno, it's all very confusing

s1341 · 2013-12-30T17:40:01Z

@marcelklehr let's try to debug together tomorrow?

marcelklehr · 2013-12-30T17:58:54Z

yea, let's see what we can do tomorrow.

It now does partial findPaths, builds the final path as it goes and requests changesets for missing bits only.

Gared · 2014-01-12T18:41:47Z

The timeslider looks very good, but it seems very slow if you have multiple revisions and/or change very fast between them. Sometimes it hangs and take a while till it jumps to the selected revision.

s1341 · 2014-01-13T14:28:19Z

There are still a few bugs and changes that need to be fixed/made. I unfortunately haven't had much time to work on this in the last couple of weeks. Hopefully I'll have some time later this week.

marcelklehr · 2014-01-25T21:13:15Z

I guess this was written with the assumption that the timeslider would run on the same page as the editor... (so, the editorbox would have to be moved down)

JohnMcLear · 2014-11-15T15:54:46Z

I guess this can be closed in favor of @marcelklehr 's pull request?

cc @s1341

JohnMcLear · 2014-12-29T01:12:34Z

Closing as we have another PR that supersedes this~- Thanks to @s1341 for his work!

s1341 added 30 commits December 17, 2013 17:35

handlers dict needs to be in AuthorizedClient; cleanup

95310da

Initial UI classes, with working partial rendering

e6a426d

The slider handle and stars render correctly. Not yet wired up to real events.

forgot to add jquery.class plugin

012cd18

without this, the timeslider and sliderui classes don't work

Working slider with mouse and events

57ed44f

Working slider class with mouse handling (click, drag) and events (change, slide). Lots of console logs still in the code. Got rid of some of the legacy code.

Steppers/Play-pause now in CSS

748b38f

Also, the steppers actually work.

Split slider out to a separate file

788a59b

add revisioncache which is destined to replace broadcast_revisions

2a63aa5

add missing require for jquery.class

488c110

wrong findPath function. corrected

a90cf74

Fix typos, add Thread class

30279e2

Add configurable interval

47a6c74

Working changesetloader (no caching)

78eb0c7

PadClient initial

6526942

Spike of revision graph

8c96c4c

Fix syntax errors

e54d63d

add missing break

88971c1

Cleanup code (jshint)

3d4b6a9

Missed one element

66c75e0

working revision/changeset graph

1eddedc

findPath works. transition requests changesets from server. processing of changesets works correctly. Only need to manage the async-ifying of transition, which is currently written as a while loop, (with a short-circuit break for debugging).

Fix reverse traversals

c634822

Make the transition algo a little more accurate.

186efe0

There are still some weird kinks in the traversal algorithm. For example, trying to go from 9 -> 19 fails very oddly.

Fix discontinuity detection in revision traversal

360bb55

Cleanup cruft. Start wiring things up

11146ce

Cleanup

f49abe6

More or less working goToRevision

3013bab

There are still some bugs in the traversal algorithm, as well as bugs being triggered in the server. Also, for some reason it looks like there is a problem with the attribute pool not containing certain elements.

Minimal working UI wireup

eef0849

Working: - location.hash - click on slider - stepper buttons - UI update (except authors) Not working: - playback - bugs in transition code

Handle is only moved when triggered event is done.

676ff3f

Sliding now triggers goToRevision.

b8591ed

Change changesetLoader to run every 200ms:wq

ccfb0e2

s1341 added 4 commits December 21, 2013 19:25

Handle new author info and revisions

805e492

Handle upstream new revisions and added users/authors.

Fixed transition algo not incrementing start

b4baba6

Added HUGE (1000 revs) bucket. There are still some libchangeset errors which need fixing.

Handle clicking slider when playing

d89babf

Fix transition bug; add scrollIntoView.

321aa05

s1341 added 4 commits December 24, 2013 19:30

Logging verbose by class

39dc862

Fix logging

f259481

missing brace

9ee80c7

Fix the bug causing the lineAssem assert

aaf0283

marcelklehr mentioned this pull request Dec 30, 2013

Add assertions for the results of CHANGESET_REQ #2054

Closed

s1341 added 3 commits December 31, 2013 18:03

transition is now much clearer.

1060544

It now does partial findPaths, builds the final path as it goes and requests changesets for missing bits only.

Commit despite bug in requestChangesets

126c83f

added logging to assist in debugging

e64c589

This was referenced Jan 23, 2014

Release v3.1.0 #2067

Closed

Release 1.4.0 #2069

Merged

marcelklehr reviewed Jan 25, 2014
View reviewed changes

marcelklehr mentioned this pull request Jan 26, 2014

Timeslider++ #2070

Closed

JohnMcLear closed this Dec 29, 2014

Uh oh!

Conversation

s1341 commented Dec 17, 2013

Uh oh!

s1341 commented Dec 22, 2013

Uh oh!

JohnMcLear commented Dec 22, 2013

Uh oh!

marcelklehr commented Dec 30, 2013

Uh oh!

marcelklehr commented Dec 30, 2013

Uh oh!

marcelklehr commented Dec 30, 2013

Uh oh!

s1341 commented Dec 30, 2013

Uh oh!

marcelklehr commented Dec 30, 2013

Uh oh!

s1341 commented Dec 30, 2013

Uh oh!

marcelklehr commented Dec 30, 2013

Uh oh!

Gared commented Jan 12, 2014

Uh oh!

s1341 commented Jan 13, 2014

Uh oh!

marcelklehr Jan 25, 2014

Choose a reason for hiding this comment

Uh oh!

JohnMcLear commented Nov 15, 2014

Uh oh!

JohnMcLear commented Dec 29, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants