Import bug fixes #3915

JohnMcLear · 2020-04-21T15:06:07Z

ace2_inner.js needs top for any console warn
Close modal after succesfull import
Proper evaluation of directDatabaseAccess as it's a string not a bool.

Still todo. Fix duplicate page issue.

cc @seballot

muxator · 2020-04-22T00:15:26Z

Does ace2_inner.js need top for only for console.warn() or for console.whatever() too?

muxator

Two problems:

the server returns directDatabaseAccess === 'undefined' as a string. And we are adapting to that. This is a blocker for the release. It has to be fixed.
to sidestep the two columns bug, we are thinking to perform a full page refresh after the import.
I might be ok with releasing with this elegant fix, but only if we at least have a hint on what can be the root cause before releasing 1.8.3. Everyone wants to move on here.

So:

1 -> blocker
2 -> acceptable if we have a plan

src/static/js/pad_impexp.js

muxator · 2020-04-22T00:29:44Z

src/static/js/pad_impexp.js

+      }
+
+      // directDatabaseAccess is a string not a boolean
+      if(directDatabaseAccess !== 'undefined'){


Do we really want to do this instead of finding the server side bug?

It was doing this before.... This isn't new code, it's just fixing it up.

Look at this. May it be here?

etherpad-lite/src/node/handler/ImportHandler.js

Line 283 in 789dd7a

var impexp = window.parent.padimpexp.handleFrameCall('" + req.directDatabaseAccess +"', '" + status + "'); \

src/static/js/pad_impexp.js

JohnMcLear · 2020-04-22T10:03:08Z

Does ace2_inner.js need top for only for console.warn() or for console.whatever() too?

.whatever too

muxator · 2020-04-22T20:08:13Z

.whatever too

ok, thanks. I'll commit a change for the whole ace2_inner.js. There are mixed console.whatever() statements there, and people may get confused.

…e.*" For reference, see: ether#3915 (comment)

…e.*" For reference, see: #3915 (comment)

JohnMcLear · 2020-04-23T12:22:04Z


jose@server:~/etheretherpad-lite$ git bisect bad
5fd6aeeea62674cecf997421546a675d91cf45ef is the first bad commit
commit 5fd6aeeea62674cecf997421546a675d91cf45ef
Author: Sebastian Castro <sebastian.castro@protonmail.com>
Date:   Thu Apr 2 14:36:49 2020 +0200

    css: refactor element positioning

    No more javascript to change css properties
    Remove a number of useless tables
    Try to stop positioning elements with absolute, but use flex-boxes instead

    Adds comment to pad template, and move popups and chatbox inside editorcontainerbox (so absolute positioning is straightforward)

    Make the design more consistent: always use base color, font-family and font-size. USe relative font size if necessary (.9rem instead of 11px for example)

    Remove two columns in the popups, just use one column

    Remove css meant to support old browser (like -webkit-box-shadow, -moz-box-shadow). Those css rules are quite common now, and If we want to support very old browser, we should use clean-css or other tools to add them automatically

:040000 040000 978d82573baabc336d70aa1ddf2f7de12eba090e 5d24f199c525d3cab707d13f1a89d2b736a8f4d1 M   src

5fd6aee

JohnMcLear · 2020-04-23T12:28:40Z

Fault applies on both no-skin and colibiris. It was introduced in 5fd6aee by @seballot - I expect it was the change to pad.html.

Hrm, no, not pad.html or ace.js

seballot · 2020-04-23T12:40:24Z

What is the problem here I don't understand?

JohnMcLear · 2020-04-23T12:41:37Z

What is the problem here I don't understand?

For some reason your commit (above) is causing the duplicate frame issue on .etherpad import. I used git bisect to discover the cause.

JohnMcLear · 2020-04-23T12:46:05Z

If you do git checkout 0603bf8097b6d3366d4c65435916a4e6f4c5536a which is the parent, then re-run Etherpad you will notice .etherpad file import works as expected (doesn't create two editors).

JohnMcLear · 2020-04-23T12:55:18Z

@seballot I tried reverting the changes to pad.html and ace.js thinking that they surely were the cause but that didn't fix it. I'm at a bit of a loss.. I hope I have pointed you in the right direction though if you can take a look at your code to see if it's something obvious?

This is what I used to figure out this was the offending commit: https://mclear.co.uk/2013/04/02/using-git-bisect-to-debug-etherpad-issues/

seballot · 2020-04-23T13:04:54Z

I had a look, but cannot understand either

JohnMcLear · 2020-04-23T13:32:22Z

@seballot okay well I will take another look and see if I can figure it out!

JohnMcLear · 2020-04-23T13:35:09Z

@seballot okay well I will take another look and see if I can figure it out!

JohnMcLear · 2020-04-23T15:12:05Z

@seballot okay well I will take another look and see if I can figure it out!

JohnMcLear · 2020-04-23T15:25:03Z

Figured it out. Jesus what a nasty bug.. Good job @seballot this was 100% NOT your fault. This is my fault and your "good" code highlighted my sloppy job :)

Note the two ace_outer elements... This was in the older versions.... *sighs.. I need to nuke one before bringing the other one in..

JohnMcLear · 2020-04-23T15:38:35Z

Fix #3925

seballot · 2020-04-23T18:49:24Z

ohh so you mean the two iframe have always been there after import, but as they were positioned in absolute the last created was just on top of the old one ! Funny one ah ah

JohnMcLear · 2020-04-23T18:51:11Z

Yes exactly :D *laughs in shame.

muxator · 2020-04-23T22:50:16Z

Kudos! Great job!

I am going to prepare the release during the weekend, since almost everything is ready (I have still a small problem with the doc generation, fix underway.

Just to be clear: #3925 and this PR go together, right?

JohnMcLear · 2020-04-24T19:59:08Z

3925 does the job afaik, this isn't needed? Feel free to close.

merge

muxator · 2020-04-26T01:46:46Z

Everything that was in this PR was integrated (e.g. closing of the modal after import, changes on logging in ace2_inner), or replaced with a better solution (e.g.: removing the duplicate iframe instead of reloading the page).

The only non merged change that is still contained in this PR is the following:

diff --git a/src/static/js/pad_impexp.js b/src/static/js/pad_impexp.js
--- a/src/static/js/pad_impexp.js
+++ b/src/static/js/pad_impexp.js
@@ -253,7 +253,8 @@ var padimpexp = (function()
         $('#import_export').removeClass('popup-show');
       }
 
-      if (directDatabaseAccess) {
+      // directDatabaseAccess is a string not a boolean
+      if (directDatabaseAccess !== 'undefined') {
         // Switch to the pad without redrawing the page
         pad.switchToPad(clientVars.padId);
         $('#import_export').removeClass('popup-show');

What problem did it fix?

If it is important, a proper fix should be done, maybe looking at

etherpad-lite/src/node/handler/ImportHandler.js

Line 283 in 789dd7a

    
                   var impexp = window.parent.padimpexp.handleFrameCall('" + req.directDatabaseAccess +"', '" + status + "'); \

Closing, let's eventually open a new PR to define & fix this last issue.

JohnMcLear added 3 commits April 21, 2020 15:03

pad_impexp: close modal on succesful import

36eadb3

pad_impexp: handle the string

2b0231b

pad_impexp: refresh the page instead, ungraceful but works

136faa4

JohnMcLear changed the title ~~Import bug fixes - WIP~~ Import bug fixes Apr 21, 2020

JohnMcLear mentioned this pull request Apr 21, 2020

New FlexLayout / Browser compatibility #3907

Closed

muxator suggested changes Apr 22, 2020

View reviewed changes

muxator force-pushed the ace2_inner_warn branch from 3b91510 to efac2ef Compare April 22, 2020 01:53

muxator added this to the 1.8.3 milestone Apr 22, 2020

muxator added a commit to JohnMcLear/etherpad-lite that referenced this pull request Apr 22, 2020

ace2_inner: prefix all logging statements: "console.*" -> "top.consol…

eb95e3e

…e.*" For reference, see: ether#3915 (comment)

muxator force-pushed the ace2_inner_warn branch 2 times, most recently from c1a2a13 to 45dfff1 Compare April 22, 2020 23:39

JohnMcLear mentioned this pull request Apr 23, 2020

1.8.3 shouldn't refresh after import .etherpad file #3923

Closed

muxator added a commit that referenced this pull request Apr 23, 2020

ace2_inner: prefix all logging statements: "console.*" -> "top.consol…

1bd595f

…e.*" For reference, see: #3915 (comment)

muxator added Export/Import UI Bug labels Apr 23, 2020

muxator mentioned this pull request Apr 23, 2020

Fix duplicated iframes after importing text in a pad #3925

Merged

JohnMcLear added 2 commits April 25, 2020 14:19

Merge pull request #11 from ether/develop

fea74cc

merge

ace2_inner: reformatting, no functional changes

d5a22b2

muxator force-pushed the ace2_inner_warn branch from 45dfff1 to 136faa4 Compare April 26, 2020 01:33

muxator closed this Apr 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import bug fixes #3915

Import bug fixes #3915

JohnMcLear commented Apr 21, 2020 •

edited

Loading

muxator commented Apr 22, 2020

muxator left a comment

muxator Apr 22, 2020

JohnMcLear Apr 22, 2020

muxator Apr 22, 2020

JohnMcLear commented Apr 22, 2020

muxator commented Apr 22, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020 •

edited

Loading

seballot commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

seballot commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

seballot commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

muxator commented Apr 23, 2020 •

edited

Loading

JohnMcLear commented Apr 24, 2020

muxator commented Apr 26, 2020

Import bug fixes #3915

Import bug fixes #3915

Conversation

JohnMcLear commented Apr 21, 2020 • edited Loading

muxator commented Apr 22, 2020

muxator left a comment

Choose a reason for hiding this comment

muxator Apr 22, 2020

Choose a reason for hiding this comment

JohnMcLear Apr 22, 2020

Choose a reason for hiding this comment

muxator Apr 22, 2020

Choose a reason for hiding this comment

JohnMcLear commented Apr 22, 2020

muxator commented Apr 22, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020 • edited Loading

seballot commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

seballot commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

seballot commented Apr 23, 2020

JohnMcLear commented Apr 23, 2020

muxator commented Apr 23, 2020 • edited Loading

JohnMcLear commented Apr 24, 2020

muxator commented Apr 26, 2020

JohnMcLear commented Apr 21, 2020 •

edited

Loading

JohnMcLear commented Apr 23, 2020 •

edited

Loading

muxator commented Apr 23, 2020 •

edited

Loading