Add playback scaling #65

sabbaka · 2017-11-27T16:04:32Z

Use css transform property

spbnick · 2017-11-28T14:02:22Z

pkg/session_recording/player.jsx

@@ -620,6 +634,9 @@

                    /* Skip packets we don't output */
                    if (!pkt.is_io || !pkt.is_output) {
+                        this.setState({cols: pkt.width, rows: pkt.height});
+                        this.state.term.resize(pkt.width, pkt.height);
+                        this._transform();


This will also execute on user input, which it shouldn't do. I think you need to rework this condition to only execute your part on !pkt.is_io.

spbnick · 2017-11-30T08:58:39Z

Thank you, Kyrill. I tried this right now and it looks great. There is one problem, however: the terminal width seems to be constrained by less than the container width, if the browser view zoom is not 100%. Can anything be done about this? Here's a screenshot:

sabbaka · 2017-11-30T09:14:58Z

Well, I have one idea how this can be fixed - it's to use font-size in view point size and then transform, I'll check this.

…

On Thu, Nov 30, 2017 at 9:58 AM, Nikolai Kondrashov < ***@***.***> wrote: Thank you, Kyrill. I tried this right now and looks great. There is one problem, however: the terminal width seems to be constrained by less than the container width, if the browser view zoom is not 100%. Can anything be done about this? Here's a screenshot: [image: terminal_width_constrained] <https://user-images.githubusercontent.com/549246/33422143-572d6d14-d5bd-11e7-9d00-b11802ea888d.png> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFTQ5UeQHbbj4PJ3DM25qVdws3d67puAks5s7m4_gaJpZM4Qr3qh> .

-- Kyrylo Gliebov Software Engineer Red Hat Czech

sabbaka · 2017-12-05T15:11:21Z

In regards for problem with extra output - so, this strange symbols are logged which were not presented upon recording are added to output and are causing problems:

Those numbers 1566 and 1564 is difference between playback position and packet length ( because of this symbols ).
I've tested same code on Cockpit's side and updated tlog version ( build for Fedora 27 ) with mc - and there was this problem, though if I remove "throw" it works, but with this symbol breaking output.
If I use top to test resizing - playback works smoothly from end to start without problems.

sabbaka · 2017-12-05T16:31:49Z

As for matching size of the container, it was not a problem at all, only thing why it was appearing, that container has fluid width ( according to window size ) and I have had a static value of 630px, so now I've changed this to get actual wrapper's width, this will allow us in future to create full-screen or resizing of container features.

sabbaka · 2017-12-06T14:55:16Z

https://youtu.be/9EAN6Id1YxE
So, here is what I have for now - controls are working, but content is not centered after zooming in/out - that's because I've to use tricky way to center it inside the above container.
I've put it as a separate commit also for more simple review and also I've changed a bit work with setState - I've tried to decrease amount of calls for only one, but I don't have any metrics if it actually has any effect on performance.

sabbaka · 2017-12-06T14:59:45Z

Also, I've forgot to add one more detail - I've had the idea to add feature as a hand scrolling - for example - you zoomed in and you would just use drag'n'drop to move through content and not the scrollbars, but now I'm not sure if that should be there, because that will remove ability to select text and copy it! But it is possible to create some sort of switch ( button ) between regular cursor to work with text and drag'n'drop.

spbnick · 2017-12-06T15:07:43Z

Niice, this feels much better, and it seems the issue with non-100% zoom levels is gone. Yes, I think we need to fix the non-centered zoom-in/out as it's confusing. How about we merge the part which keeps the terminal within bounds first, and then work on scrolling/zooming/panning? That way we'll have something working merged already.

Yeah, "panning" can be done with a button, although it wouldn't be very convenient to use. Perhaps we can do the button (like http://fontawesome.io/icon/hand-paper-o/), and add a hotkey for turning it on or off, and/or a hotkey which would enable panning while pressed (i.e. hold the key and drag the screen, otherwise select).

One thing that I keep noticing is that intermediate window resizes seem to disappear and window jumps through them when skipping, unless there's I/O. Perhaps the main playback routine needs fixing. Can you check that?

Regarding the extra characters, could you please provide me with the original logs, where that reproduces? You can use the journalctl command to dump the messages for the session using the same matches Cockpit uses when playing back the session. I'll try to see where there is a problem in encoding/decoding.

spbnick · 2017-12-06T15:10:17Z

Oh, and another thing: the "zoom to fit" icon seems a bit confusing to me. Do you think we can draw and/or use something like a "square under a lens" icon? E.g. something like these: https://www.iconfinder.com/search/?q=zoom+fit
Or it would be breaking our pattern of using Font Awesome?

sabbaka · 2017-12-06T17:06:55Z

Yes, for now, if you will play or skip frames zoom levels will be switched back to match container ( not preserving zoom levels basically ).
This is "broken" session:
mc_broken_session_dump.txt

sabbaka · 2017-12-06T17:08:28Z

I think we should stick with font awesome to keep overall style. I'll add hotkeys and split commits in two - for playback scaling and for controls

sabbaka · 2017-12-07T12:59:33Z

I've separated commits and now in this PR it's only commit for scaling without buttons.

spbnick

Thank you, Kyrill! This looks alright aside from the inline comments.

However, I notice that the playback area gets a scrollbar sometimes, which covers parts of the terminal, especially at extreme proportions. Can that be fixed somehow?

spbnick · 2017-12-07T13:29:09Z

pkg/session_recording/player.jsx

@@ -473,6 +474,8 @@
                paused:     true,
                /* Speed exponent */
                speedExp:   0,
+                scale: 1,
+                container_width: 630,


Could you please keep values aligned here?

spbnick · 2017-12-07T13:30:09Z

pkg/session_recording/player.jsx

+        _transform(width, height) {
+            var relation = Math.min(
+              this.state.container_width / this.state.term.element.offsetWidth,
+              290 / this.state.term.element.offsetHeight


What is this 290? Could you please use a constant with a name which would give a clue instead?

spbnick · 2017-12-07T13:31:15Z

pkg/session_recording/player.jsx

@@ -620,6 +639,8 @@

                    /* Skip packets we don't output */
                    if (!pkt.is_io || !pkt.is_output) {
+                        this.state.term.resize(pkt.width, pkt.height);
+                        this._transform(pkt.width, pkt.height);


This is also being called on "input" packets, not only on "window" packets. You're not hitting this now, because input is not recorded by default. Please fix.

Essentially, instead of adding code here, the if condition should be changed to pkt.is_io && !pkt.is_output, and the code added instead after the "Output the packet" comment below, branching on output/window packet type. This would also fix the timing issues with window resizing.

Yes, I'm doing this now. Just missed this problem before. Thanks for clarifying, I was about to use just if (pkt.is_output)

I've made a bit different change, not sure if it's okay, but my implementation of your provided idea did not work, let's also discuss this tomorrow, probably I'm missing something.

spbnick · 2017-12-07T13:36:23Z

I think that there's value in a blinking cursor, and we can discuss that, but let's not lump that change with this one.

sabbaka · 2017-12-08T16:06:31Z

I've removed removal of blinking cursor and added ours together if/else fix from meeting.

spbnick · 2017-12-11T18:35:17Z

I took a look at this today and it feels much better. However, I see that sometimes window resize and repaint play back with a huge delay between them. I'm not sure where that comes from. Will investigate tomorrow.

spbnick · 2017-12-12T07:52:43Z

@sabbaka, I still see the old resizing code in the commit, not the one we arrived at on our meeting. Have you forgotten to commit/push something, perhaps?

sabbaka · 2017-12-12T08:53:46Z

I’ll need to check that.

On Tue, 12 Dec 2017 at 8:53, Nikolai Kondrashov ***@***.***> wrote: @sabbaka <https://github.com/sabbaka>, I still see the old resizing code in the commit, not the one we arrived at on our meeting. Have you forgotten to commit/push something, perhaps? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFTQ5UDHDSpIzHx5rd8-yLCG2aks9y-Mks5s_jDLgaJpZM4Qr3qh> .

-- Kyrylo Gliebov Software Engineer Red Hat Czech

sabbaka · 2017-12-12T08:58:22Z

Code should be fine now, it seems that I've just forgot to push.

spbnick

Apart from the three inline style comments, this looks and works great! Thank you, Kyrill.

spbnick · 2017-12-12T10:16:13Z

pkg/session_recording/player.jsx

@@ -473,8 +474,12 @@
                paused:     true,
                /* Speed exponent */
                speedExp:   0,
+                scale: 1,
+                containerWidth: 630,


Please align the new values with the rest.

spbnick · 2017-12-12T10:16:33Z

pkg/session_recording/player.jsx

@@ -532,7 +537,7 @@
                cols: this.state.cols,
                rows: this.state.rows,
                screenKeys: true,
-                useStyle: true
+                useStyle: true,


Please remove this unnecessary change.

spbnick · 2017-12-12T10:17:31Z

pkg/session_recording/player.jsx

@@ -617,9 +637,8 @@
                        this.awaitPacket();
                        return;
                    }
-


Please remove this unnecessary change.

sabbaka · 2017-12-12T10:40:13Z

I've made changes, but I'm not sure if I understand how to align correctly.

spbnick · 2017-12-12T10:44:33Z

Thank you. Just make them all appear on the same column. Like this:

    cols:           80,
    rows:           25,
    title:          _("Player"),
    term:           null,
    paused:         true,
    /* Speed exponent */
    speedExp:       0,
    containerWidth: 630,
    scale:          1

Use css transform property

spbnick · 2017-12-13T08:33:17Z

Thanks a lot, looks and works great! Can you do a demo on today's meeting?

Another thing: could you please leave a comment when you're done fixing up things in response to review? I just don't get any updates from GitHub when you're force-pushing over a commit, without adding a new one. And even when you add a new one, it would be great to get a comment from you saying you're done.

sabbaka · 2017-12-13T09:33:22Z

Yes, I will do the demo.
Pardon, just missed that yesterday.

spbnick reviewed Nov 28, 2017

View reviewed changes

sabbaka force-pushed the scribery branch from 6479146 to 1bb0fbc Compare November 28, 2017 14:35

sabbaka force-pushed the scribery branch from 1bb0fbc to cb107c2 Compare December 5, 2017 16:30

sabbaka force-pushed the scribery branch from cb107c2 to 523d6d3 Compare December 5, 2017 16:54

sabbaka force-pushed the scribery branch from be32d69 to 0db55de Compare December 7, 2017 12:58

spbnick requested changes Dec 7, 2017

View reviewed changes

sabbaka force-pushed the scribery branch 3 times, most recently from c2ec90f to 9da7540 Compare December 8, 2017 12:34

sabbaka force-pushed the scribery branch from 9da7540 to 414cdec Compare December 12, 2017 08:57

spbnick requested changes Dec 12, 2017

View reviewed changes

sabbaka force-pushed the scribery branch from 414cdec to c11a7bb Compare December 12, 2017 10:39

Add playback scaling

aa98508

Use css transform property

sabbaka force-pushed the scribery branch from c11a7bb to aa98508 Compare December 12, 2017 10:59

spbnick approved these changes Dec 13, 2017

View reviewed changes

spbnick merged commit 8579db4 into Scribery:scribery Dec 13, 2017

sabbaka mentioned this pull request Apr 12, 2018

Implement resizing terminal to recorded window size #53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add playback scaling #65

Add playback scaling #65

sabbaka commented Nov 27, 2017

spbnick Nov 28, 2017

spbnick commented Nov 30, 2017 •

edited

Loading

sabbaka commented Nov 30, 2017 via email

sabbaka commented Dec 5, 2017

sabbaka commented Dec 5, 2017 •

edited

Loading

sabbaka commented Dec 6, 2017

sabbaka commented Dec 6, 2017

spbnick commented Dec 6, 2017

spbnick commented Dec 6, 2017

sabbaka commented Dec 6, 2017 •

edited

Loading

sabbaka commented Dec 6, 2017

sabbaka commented Dec 7, 2017

spbnick left a comment

spbnick Dec 7, 2017

spbnick Dec 7, 2017

spbnick Dec 7, 2017

spbnick Dec 7, 2017

sabbaka Dec 7, 2017

sabbaka Dec 7, 2017

spbnick commented Dec 7, 2017

sabbaka commented Dec 8, 2017 •

edited

Loading

spbnick commented Dec 11, 2017 •

edited

Loading

spbnick commented Dec 12, 2017

sabbaka commented Dec 12, 2017 via email

sabbaka commented Dec 12, 2017

spbnick left a comment

spbnick Dec 12, 2017

spbnick Dec 12, 2017

spbnick Dec 12, 2017

sabbaka commented Dec 12, 2017

spbnick commented Dec 12, 2017

spbnick commented Dec 13, 2017

sabbaka commented Dec 13, 2017

Add playback scaling #65

Add playback scaling #65

Conversation

sabbaka commented Nov 27, 2017

Choose a reason for hiding this comment

spbnick commented Nov 30, 2017 • edited Loading

sabbaka commented Nov 30, 2017 via email

sabbaka commented Dec 5, 2017

sabbaka commented Dec 5, 2017 • edited Loading

sabbaka commented Dec 6, 2017

sabbaka commented Dec 6, 2017

spbnick commented Dec 6, 2017

spbnick commented Dec 6, 2017

sabbaka commented Dec 6, 2017 • edited Loading

sabbaka commented Dec 6, 2017

sabbaka commented Dec 7, 2017

spbnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spbnick commented Dec 7, 2017

sabbaka commented Dec 8, 2017 • edited Loading

spbnick commented Dec 11, 2017 • edited Loading

spbnick commented Dec 12, 2017

sabbaka commented Dec 12, 2017 via email

sabbaka commented Dec 12, 2017

spbnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabbaka commented Dec 12, 2017

spbnick commented Dec 12, 2017

spbnick commented Dec 13, 2017

sabbaka commented Dec 13, 2017

spbnick commented Nov 30, 2017 •

edited

Loading

sabbaka commented Dec 5, 2017 •

edited

Loading

sabbaka commented Dec 6, 2017 •

edited

Loading

sabbaka commented Dec 8, 2017 •

edited

Loading

spbnick commented Dec 11, 2017 •

edited

Loading