Timeslider++ - Githubissues

marcelklehr commented 10 years ago

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...

marcelklehr commented 10 years ago

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?

Gared commented 10 years ago

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 commented 10 years ago

Fixed the handler bug.

marcelklehr commented 10 years ago

Note to self: https://github.com/ether/etherpad-lite/blob/efbbd58216dd34f8767e447f9ca23d8149ea4ba5/src/static/js/revisionslider.js#L149

s1341 commented 10 years ago

@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 ;)

marcelklehr commented 10 years ago

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

marcelklehr commented 10 years ago

@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.

JohnMcLear commented 10 years ago

Woop :)

marcelklehr commented 10 years ago

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

JohnMcLear commented 10 years ago

woop will test today

JohnMcLear commented 10 years ago

I put this branch live on beta.etherpad.org

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

JohnMcLear commented 10 years ago

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..

marcelklehr commented 10 years ago

could you turn off minify?

JohnMcLear commented 10 years ago

done

marcelklehr commented 10 years ago

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) :)

marcelklehr commented 10 years ago

Disabled merging for now, try git pull

Gared commented 10 years ago

@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)

s1341 commented 10 years ago

@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.

marcelklehr commented 10 years ago

@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...

marcelklehr commented 10 years ago

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

Gared commented 10 years ago

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...

michaelkarrer81 commented 10 years ago

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)

marcelklehr commented 10 years ago

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. ;)

marcelklehr commented 10 years ago

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

marcelklehr commented 10 years ago

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!

JohnMcLear commented 10 years ago

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.

marcelklehr commented 10 years ago

1) Long story (see https://github.com/ether/etherpad-lite/pull/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 commented 9 years ago

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.

JohnMcLear commented 9 years ago

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

JohnMcLear commented 9 years ago

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

JohnMcLear commented 9 years ago

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 ?

JohnMcLear commented 9 years ago

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 https://github.com/ether/etherpad-lite/pull/2070#issuecomment-63180032

This with some work will be ready to merge :)

JohnMcLear commented 9 years ago

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

marcelklehr commented 9 years ago

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

JohnMcLear commented 9 years ago

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

JohnMcLear commented 9 years ago

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

marcelklehr commented 9 years ago

No, I won't be working on this. I'm sorry.

JohnMcLear commented 9 years ago

No worries :)

ether / etherpad-lite

Timeslider++ #2070