Timeslider++ #2070

Closed
wants to merge 76 commits into
from

Conversation

Projects
None yet
5 participants
Owner

marcelklehr commented Jan 26, 2014

These are my fixes for @s1341's new timeslider (see #2040).

The first commit essentially introduces a new path calculator that computes a path from one revision to a new one (early version of that here: https://gist.github.com/marcelklehr/8208237), the rest of the changes are collateral to this: I stripped out partialTransition for example and reduced some other parts, too.

My second commit addresses #2065 as well as the OccupyChar issue. I hope it doesn't cause any bugs, accidentally.

Lastly, there are still a few soar spots in the timeslider codez that should be addresses (e.g. try going from rev#36 to rev#25, i think), so.. NOT READY yet.

Update: see below...

s1341 and others added some commits Nov 23, 2013

@s1341 s1341 Introduce timeslider object
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.
e55ef0f
@s1341 s1341 handlers dict needs to be in AuthorizedClient; cleanup 95310da
@s1341 s1341 Initial UI classes, with working partial rendering
The slider handle and stars render correctly. Not yet wired up to real
events.
e6a426d
@s1341 s1341 forgot to add jquery.class plugin
without this, the timeslider and sliderui classes don't work
012cd18
@s1341 s1341 Working slider with mouse and events
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.
57ed44f
@s1341 s1341 Steppers/Play-pause now in CSS
Also, the steppers actually work.
748b38f
@s1341 s1341 Split slider out to a separate file 788a59b
@s1341 s1341 add revisioncache which is destined to replace broadcast_revisions 2a63aa5
@s1341 s1341 add missing require for jquery.class 488c110
@s1341 s1341 wrong findPath function. corrected a90cf74
@s1341 s1341 Fix typos, add Thread class 30279e2
@s1341 s1341 Add configurable interval 47a6c74
@s1341 s1341 Working changesetloader (no caching) 78eb0c7
@s1341 s1341 PadClient initial 6526942
@s1341 s1341 Spike of revision graph 8c96c4c
@s1341 s1341 Fix syntax errors e54d63d
@s1341 s1341 add missing break 88971c1
@s1341 s1341 Cleanup code (jshint) 3d4b6a9
@s1341 s1341 Missed one element 66c75e0
@s1341 s1341 working revision/changeset graph
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).
1eddedc
@s1341 s1341 Fix reverse traversals c634822
@s1341 s1341 Make the transition algo a little more accurate.
There are still some weird kinks in the traversal algorithm. For
example, trying to go from 9 -> 19 fails very oddly.
186efe0
@s1341 s1341 Fix discontinuity detection in revision traversal 360bb55
@s1341 s1341 Cleanup cruft. Start wiring things up 11146ce
@s1341 s1341 Cleanup f49abe6
@s1341 s1341 More or less working goToRevision
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.
3013bab
@s1341 s1341 Minimal working UI wireup
Working:
  - location.hash
  - click on slider
  - stepper buttons
  - UI update (except authors)
Not working:
  - playback
  - bugs in transition code
eef0849
@s1341 s1341 Handle is only moved when triggered event is done. 676ff3f
@s1341 s1341 Sliding now triggers goToRevision. b8591ed
@s1341 s1341 Change changesetLoader to run every 200ms:wq ccfb0e2
@s1341 s1341 Play/Pause working. ab9b55d
@s1341 s1341 Don't process clicks on steppers if disabled 14b2af4
@s1341 s1341 Fix top of editorcontainer so that it isn't behind timeslider 541f0e5
@s1341 s1341 Clicking steppers should pause playback 13f4a71
@s1341 s1341 Allow compose or apply to be used in GoToRevision
Controlled on the static PadClient.USE_COMPOSE variable.
a5eced9
@s1341 s1341 Cleanup; add getHeadRevision function to connection 275c26f
@s1341 s1341 Workaround for granularity issue.
Just return the granularity discovered to be empty from findPath and
provide it to requestChangesets. This is very sub-optimal, as it will
almost always result in granularity==1.
1c6012b
@s1341 s1341 Real fix for the traversal/granularity bug.
The algo could still use improvements. In addition, the order of
execution of the callbacks means that the lower granularities will
always be available first, even if higher granularities were requested.

Note that there are some strange libchangeset errors now.
ed3e88b
@s1341 s1341 Implement authorinfo; split RevisionSlider out 3c0bd57
@s1341 s1341 Handle new author info and revisions
Handle upstream new revisions and added users/authors.
805e492
@s1341 s1341 Fixed transition algo not incrementing start
Added HUGE (1000 revs) bucket. There are still some libchangeset errors
which need fixing.
b4baba6
@s1341 s1341 Handle clicking slider when playing d89babf
@s1341 s1341 Fix transition bug; add scrollIntoView. 321aa05
@s1341 s1341 Logging verbose by class 39dc862
@s1341 s1341 Fix logging f259481
@s1341 s1341 missing brace 9ee80c7
@s1341 s1341 Fix the bug causing the lineAssem assert aaf0283
@s1341 s1341 transition is now much clearer.
It now does partial findPaths, builds the final path as it goes  and requests changesets for missing bits only.
1060544
@s1341 s1341 Commit despite bug in requestChangesets 126c83f
@s1341 s1341 added logging to assist in debugging e64c589
@marcelklehr marcelklehr Simplify RevisionCache
switch to deterministic path finder and simplify changeset fetching
8e2e9d7
@marcelklehr marcelklehr Fix server-side revision numbering 88c97a9

marcelklehr referenced this pull request Jan 26, 2014

Merged

Release 1.4.0 #2069

6 of 8 tasks complete
Owner

marcelklehr commented Jan 26, 2014

All issues resolved. Works like a charm and even looks good now :)

@s1341: Author colors in the content would be nice, still. Any thoughts about how to achieve this?

Member

Gared commented Jan 26, 2014

It would be nice if the authors will be shown that are affected to the selected pad revision. Now all authors are shown. In the old etherpad the behaviour is as I expected.
Also it would be nice to see always the last version of a pad if you have selected the last revision (live timeslider).
I think there is also a bug: I can not select the last revision, because this is a few pixel outside of the timeslider itself.

@marcelklehr marcelklehr Timeslider UI: Improve UX for slider handle
Round() slider click to next step instead of floor()ing
Allow people to click on last revision
Position the handle correctly at beginning and end
c3b8e45
Owner

marcelklehr commented Jan 26, 2014

Fixed the handler bug.

Contributor

s1341 commented Jan 27, 2014

@marcelklehr Wow! Thanks for picking up my slack on this. RL got the best of me for a few weeks. I'm still not able to dedicate any real time to this. Looks like you've managed to fix a bunch of my bugs. I'm hoping my refactor was at least useful as a base to you ;)

Owner

marcelklehr commented Jan 27, 2014

Definitely! Fixing these was a breeze. I love the new implementation, you did a great job revamping timeslider!

@marcelklehr marcelklehr commented on an outdated diff Jan 27, 2014

src/static/js/revisioncache.js
+ var path = []
+ , delta = to - from
+ , directionLeft = delta < 0
+ , adelta = Math.abs(delta)
+ , sign = delta / adelta
+ , granularity
+ , next, shortcut
+ , max = this.head_revision.revnum
+
+ if(delta == 0) return path
+
+ for(var g=0; g < Revision.granularities.length; g++) {
+ granularity = Revision.granularities[g]
+ if(granularity > max) continue;
+ if(granularity <= adelta
+ || Math.round((adelta % granularity)/granularity) == 1 // in this case it's faster to overshoot the target and then go back
@marcelklehr

marcelklehr Jan 27, 2014

Owner

aah, this is stupid. Instead it should check:
Math.round(from / granularity)*granularity != Math.round(to /granulariy)*granularity

@marcelklehr marcelklehr Timeslider: Fix calcPath()'s smart overshooting feature
Don't overshoot with higher granularity if the granularity mark
is in between 'from' and 'to'.
e.g. not 34->33->32->31->30->20->21->22->23->24->25->26
(instead 34->33->32->31->30->29->28->27->26)
c0307bd
Owner

marcelklehr commented Jan 28, 2014

@Gared Thanks for the feedback!

I fixed the slider handle. It's now positioned correctly at start and end and you can also click on the end directly.

If you're viewing the last revision and you're in play mode, new changes will be instantly visible as the happen. If you're not in play mode, then you'll just see the slider handle adjust (making room for new changes).

Additionally, the toolbar displays only authors that are visible in the current revision's content and highlights the author responsible for the change that led to the current revision.

@JohnMcLear: Also, I found out why the browser was crashing on me sometimes: I made a mistake in the algorithm that computes the path from one revision to another one. It would recurse until the browser crashed in certain scenarios ;)

Also, timeslider still works even when you go offline.

Owner

JohnMcLear commented Jan 28, 2014

Woop :)

Owner

marcelklehr commented Jan 28, 2014

Okies, I'd say it's ready to pull now. please test and provide feedback.

Owner

JohnMcLear commented Jan 28, 2014

woop will test today

Owner

JohnMcLear commented Jan 28, 2014

I put this branch live on beta.etherpad.org

http://beta.etherpad.org/test/timeslider

Owner

JohnMcLear commented Jan 28, 2014

Observations:

  • Play button doesn't work, if you hit play at the end of a document it should either play the document in real time or go to the start of teh document
  • Clicking to go back in time doesn't work consistantly
  • Goin back to a revision then hitting play seems to play from the last known good state

For some reason there is a ugly animation that makes the timeslider expand out, this doesn't follow the UI of etherpad so should be removed..

Clicking the forward/back buttons seem to work quite well assuming you are on a loaded revision /state

Imho so far from a users point of view there isn't any improvement from the status quo..

Owner

marcelklehr commented Jan 28, 2014

could you turn off minify?

Owner

JohnMcLear commented Jan 28, 2014

done

Owner

marcelklehr commented Jan 28, 2014

I'm not sure what "ugly animation" you're talking about, but all your other observations seem to be due to the fact that libchangeset has some problems merging changesets (at least on /p/test) :)

Owner

marcelklehr commented Jan 28, 2014

Disabled merging for now, try git pull

Member

Gared commented Jan 28, 2014

@marcelklehr Great work :-)
I started testing right now and found a bug when on wrong author color.
To reproduce it:

  • Open in one browser the timeslider and in another browser the pad
  • Right something in the pad
  • Select a newer version in the timeslider and it will show you the added text in another author color
    (Maybe you have to write with two users to reproduce it)
Contributor

s1341 commented Jan 28, 2014

@marcelklehr. Thanks again for taking this forwards. Glad the refactored code makes things easier. I'm only sorry I wasn't able to manage this on my own.

Owner

marcelklehr commented Jan 28, 2014

@s1341 No worries. This is what makes Open source so great :)
@JohnMcLear For some reason some attributes don't seem to exist in the attribute pool we got from the server.

17:58:59.750 "[transition] 33 -> 34, changeset: Z:g>0*2|2=f$"    require-kernel.js:247
17:58:59.751 TypeError: pair is undefined    timeslider.js:3125
17:59:27.860 <_this.padClient.apool.getAttrib(2)
17:59:27.864 > undefined

@Gared Confirmed. So something goes wrong when new changes are requested and stored... Strangely we receive the changesets like that from the server, it seems. Try reloading the timeslider, step back a few revisions and then go forward again: Somehow the changes have changed their author...

Owner

marcelklehr commented Jan 28, 2014

I think the last two issues might be related. inconsistent attribute state. Bad thing. I'm at a loss to explain this, right now...

Member

Gared commented Jan 28, 2014

Other bugs:

  • "Live changes" on timeslider doesn't recognize changes like underline and strikethrough. Also all changes on lists won't be handled correct. Same by deletion of author colors.
  • star of "mark as saved revision" won't be displayed until reload of timeslider
  • attributes on delted text like strikethrough are set on text that is written on the same position (no page reload of timeslider between deletion and added text)
  • Also I got this message: "Ooops! All possibility sources drained. Input is likely to be invalid." I think this is a debug of you, isn't it?
  • If a star is between the version you want to go to and from where you come then you'll get a javascript error message (Uncaught TypeError: Cannot read property '0' of undefined). (not in every case. Can't reproduce it yet)
  • Adding one character will not update timeslider

Also I agree with John that there is an issue that sometimes the timeslider hangs and it take some time until anything happens (sometimes there is a javascript error in console, sometimes not). I think this happens if you make some "bad changes" (see the first bug in my list above).
Sorry for reporting so much errors...

hm i am new to etherpad-lite - does this mean that the timeslider is currently broken? completely ?!? i am on version 1.14 (sorry if this is the wrong place to ask this)

Owner

marcelklehr commented Feb 13, 2014

There are (and have been for quite a while) some problems with the timeslider in the master branch. This (as well as #2040) is a complete re-implementation of the timeslider. All bugs mentioned in this thread refer to the version proposed here. ;)

Owner

marcelklehr commented Feb 13, 2014

http://ricostacruz.com/nprogress/ might be nice to have since loading is apparently quite slow.

Owner

marcelklehr commented Feb 27, 2014

I would very much appreciate some help right now. Please have a go at testing this everyone (use attributes like lists and stuff), and please help sort out why they're not displayed correctly. Thank you!

Owner

JohnMcLear commented Mar 2, 2014

Bug1) Revision 0 doesn't show the initial pad contents. To replicate:

  • Create a new pad
  • Type additional content
  • Create 5 or so revisions.

Revision 0 is incorrect.

Bug 2) Play doesn't work
Hitting play on a pad with 20 revisions does nothing (no error etc), data does display in console. Hitting pause twice starts the pad playing fine.

Owner

marcelklehr commented Mar 7, 2014

  1. Long story (see #2054) tl;dr: There are different rev counts. I tried to to avoid getting insane about them and chose to deal with one only. The result is that rev0 is '\n' in the timeslider (however, rev0 in the db is the result of applying _cs_0 on '\n'). I don't know if my current approach is a good idea, but it works.

  2. This is probably because I changed the play button to always play, so if it arrives at the end it'll just keep playing. Rationale: Hit play and see edits in real-time as they happen on the pad.

JohnMcLear self-assigned this Nov 15, 2014

Owner

JohnMcLear commented Nov 15, 2014

The UX is really broken but the functionaltiy seems pretty solid. @0ip @luto did you guys wanna look at the UI/X on this and get it tight? Note the UI has been heavily modified since so you will want to rebase onto develop.

Owner

JohnMcLear commented Nov 15, 2014

@marcelklehr the version 0 is still showing nothing. Why not just do an if content == "" show rev1 instead of rev0?

Owner

JohnMcLear commented Nov 15, 2014

/tests/frontend/?grep=timeslider_labels.js and /tests/frontend/?grep=timeslider_revisions.js fail -- I will sort these.

Owner

JohnMcLear commented Nov 15, 2014

There is no ability to export a given revision any more.

There is no button to go back to the pad.

Is this expected behavior @marcelklehr ?

Owner

JohnMcLear commented Nov 15, 2014

The test it("jumps to a revision given in the url", function(done) { fails intentionally as it attempt to get teh contents at rev0 which is currently incorrect as per #2070 (comment)

This with some work will be ready to merge :)

JohnMcLear removed their assignment Nov 15, 2014

Owner

JohnMcLear commented Nov 26, 2014

@marcelklehr I rebased this so bugfixing for you should be easier :)

Owner

marcelklehr commented Nov 29, 2014

the timeslider bar is gone now, is that intentional? Also, something's messing up the attributes, I can't wrap my head around this right now

Owner

JohnMcLear commented Nov 29, 2014

Yeah the bar gone is intentional until we figure out a good ui,ux

Owner

JohnMcLear commented Mar 31, 2015

Closing as it's been neglected.. I dunno if @marcelklehr wants to take this on again, doesn't seem that way!

JohnMcLear closed this Mar 31, 2015

Owner

marcelklehr commented Apr 2, 2015

Owner

JohnMcLear commented Apr 2, 2015

No worries :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment