Refactored section edit #2220

lpaulsen93 · 2018-01-04T13:44:16Z

The data for the section edit button is now passed as an assoziative array which is
encoded in the '##' placeholder as an JSON array.

This also already includes the fix for issue #741.

This PR needs to be reviewed and it needs to be examined which plugins might need an adjustment.

lpaulsen93 · 2018-01-04T13:46:21Z

At the moment the following things (still) work:

unit tests run without error
section edit works:
- including the older fix for section edit with two sections having the same name/heading
- issue Wrong code download in preview of section edit #741
editing tables works (edittable plugin)

lpaulsen93 · 2018-01-04T13:55:58Z

unit tests run without error

Wrong, some tests fail. I tested on a wrong directory, sorry. Will fix this.

lpaulsen93 · 2018-01-04T14:48:16Z

I got the following plugins installed:
acl, authldap, authplain, bookcreator, cloud, creole, dw2pdf, feed, include, linkback, miniblog, pageimage, popularity, revert, text, authmysql, avatar, button, color, definitionlist, edittable, fields, mathjax, note, pagelist, publish, safefnrecode, tablewidth, typography, authpdo, backlinks, changemarks, columns, dir, emphasis, filelist, info, mathpublish, numberedheadings, pagelistng, randominc, searchstats, tag, usermanager, anewssystem, authpgsql, blockquote, chem, comment, discussion, extension, flattr, jabber, meta, odt, plugincheck, readability, styling, task, var, authad, blog, clippy, config, disqus, exttab3, folded, keyboard, mimetex, pagebreak, pluginrepo, switchpanel, testing, wrap

From that list I see the following plugins which would need an adjustment:

wrap
text
include

splitbrain

This looks good already. I left a few comments, I think we can clean up the start/range/end handling a little.

splitbrain · 2018-01-06T09:22:17Z

inc/html.php

-    if (!empty($matches['name'])) {
-        $data['name'] = $matches['name'];
-    }
+    $data = json_decode($matches[1], true);


We probably need to catch some error here to avoid problems where for some reason the old syntax is still used.

I don't think json_decode throws proper exceptions, so we probably need to suppress the error and check if $data is null.

Alternatively, we could adjust the pattern matching the syntax to include the opening and closing {} of the JSON. That way we would ignore all old syntax.

Is it likely that such errors can happen? I mean it is encoded in 'finishSectionEdit()', inc/parser/xhtml.php and decoded in 'html_secedit()', inc/html.php. I assume it could only happen if alternative renderers for HTML are used. Otherwise it is only DokuWiki core internal.

Plugins might write their own section edit syntax to the HTML output without ever going through finishSectionEdit(). I don't know if any actually do though. I guess the easiest fix is to simply let the pattern look for the JSON delimiters.

Ok, I did both: added the delimiters and checked for NULL anyway as a matching pattern alone cannot guarantee a proper JSON encoding.

splitbrain · 2018-01-06T09:32:49Z

inc/parser/xhtml.php

     * @param int    $start  The byte position for the edit start
+     * @param array  $data   Assoziative array with section data:


typo: Assoziative -> Associative

splitbrain · 2018-01-06T09:34:08Z

inc/parser/xhtml.php

+     *                       Key 'hid': header id
+     *                       Key 'codeblockOffset': actual code block index
+     *                       Key 'start': reserved, do not use
+     *                       Key 'range': reserved, do not use


I don't think these should be marked as reserved. Actually they are required, aren't they?

I also wonder about having start and range. Shouldn't we have start and end instead?

The $start value is passed to startSectionEdit() as an extra parameter. The $end value is given to finishSectionEdit() as an extra parameter. In finishSectionEdit() finally the saved data['start'] value and given $end value are used to build the range. The range is the important field that is sent on clicking on the section edit button. If that is not found then the section edit will not work. So, start and end are only temporary values and I thought it is better to put the calculation of the range into startSectionEdit() and finishSectionEdit() instead of having the caller calculating and assigning the range to the array. That means the keys start and range will always be overwritten. That's why I thought it would be good to mention them as reserved.

I see. Maybe we can get the gist of that explanation into the doc block? Like:

'start': set in startSectionEdit(), do not set yourself 'range': calculated from 'start' and $key in finishSectionEdit(), do not set yourself

Something like that. I'm open for improvements on that.

Sounds good, have used it.

splitbrain · 2018-01-06T09:44:02Z

inc/parser/xhtml.php

-    public function startSectionEdit($start, $type, $title = null, $hid = null) {
-        $this->sectionedits[] = array(++$this->lastsecid, $start, $type, $title, $hid);
-        return 'sectionedit'.$this->lastsecid;
+    public function startSectionEdit($start, $data) {


we probably need to check if $data is an array - old plugins may supply a type instead

Ok, should we output a msg() in such a case? Or a dbg() message?

let's do a msg()

I'm currently trying to fix the Wrap plugin which is affected by this change. How can that be done in a backwards-compatible way?

@selfthinker can you open a new ticket or mailing list discussion. Comments on a already closed PR get lost too easily.

To answer your question, you could inspect the SECEDIT_PATTERN definition maybe?

@micgro42 fixed it and made it backwards compatible, and I opened a new issue for missing documentation (#2290).

lpaulsen93 · 2018-01-24T20:52:12Z

@splitbrain: let me know if you want to merge it, then I can squash the commits to one single commit before the merge.

lpaulsen93 · 2018-02-15T18:22:09Z

@splitbrain: ping. Do you request any more changes to this? Let me now if it can be merged then I can squash the commits together before merging.

splitbrain · 2018-02-15T18:29:01Z

squash ahead. there will be minor things only, if any

The data for the section edit button is now passed as an assoziative array which is encoded in the '##' placeholder as an JSON array.

lpaulsen93 · 2018-02-15T18:54:49Z

Done.

The signature of startSectionEdit was changed in dokuwiki/dokuwiki#2220 in a non backwards compatible way. This adjusts for that change in a way that should still work for pre-Greebo versions.

splitbrain reviewed Jan 6, 2018

View reviewed changes

splitbrain added the Refactoring ♻ label Jan 24, 2018

splitbrain added this to the 🐱 Greebo milestone Jan 24, 2018

splitbrain mentioned this pull request Jan 24, 2018

Explicitly transmit codeblock offset accross workflow "Show => Section edit => Preview". #2204

Closed

Refactored section edit

ec57f11

The data for the section edit button is now passed as an assoziative array which is encoded in the '##' placeholder as an JSON array.

lpaulsen93 force-pushed the sectioneditrefactor branch from 0fc5659 to ec57f11 Compare February 15, 2018 18:52

splitbrain merged commit dfeb484 into master Mar 6, 2018

splitbrain deleted the sectioneditrefactor branch March 6, 2018 18:18

micgro42 mentioned this pull request Mar 22, 2018

account for startSectionEdit signature change (backwards compatible) selfthinker/dokuwiki_plugin_wrap#162

Merged

selfthinker mentioned this pull request Mar 25, 2018

startSectionEdit changes missing documentation #2290

Closed

selfthinker mentioned this pull request Apr 8, 2018

Section edit changes produce invalid HTML in Wrap plugin #2304

Closed

micgro42 mentioned this pull request Apr 9, 2018

Fix section edit error message splitbrain/dokuwiki-plugin-data#215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored section edit #2220

Refactored section edit #2220

lpaulsen93 commented Jan 4, 2018

lpaulsen93 commented Jan 4, 2018

lpaulsen93 commented Jan 4, 2018

lpaulsen93 commented Jan 4, 2018

splitbrain left a comment

splitbrain Jan 6, 2018

splitbrain Jan 6, 2018

lpaulsen93 Jan 6, 2018

splitbrain Jan 6, 2018

lpaulsen93 Jan 6, 2018

splitbrain Jan 6, 2018

lpaulsen93 Jan 6, 2018

splitbrain Jan 6, 2018

splitbrain Jan 6, 2018

lpaulsen93 Jan 6, 2018

splitbrain Jan 24, 2018

lpaulsen93 Jan 24, 2018

splitbrain Jan 6, 2018

lpaulsen93 Jan 6, 2018

splitbrain Jan 24, 2018

lpaulsen93 Jan 24, 2018

selfthinker Mar 18, 2018

splitbrain Mar 19, 2018

selfthinker Mar 25, 2018

lpaulsen93 commented Jan 24, 2018

lpaulsen93 commented Feb 15, 2018

splitbrain commented Feb 15, 2018

lpaulsen93 commented Feb 15, 2018

		* @param int $start The byte position for the edit start
		* @param array $data Assoziative array with section data:

Refactored section edit #2220

Refactored section edit #2220

Conversation

lpaulsen93 commented Jan 4, 2018

lpaulsen93 commented Jan 4, 2018

lpaulsen93 commented Jan 4, 2018

lpaulsen93 commented Jan 4, 2018

splitbrain 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

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

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

lpaulsen93 commented Jan 24, 2018

lpaulsen93 commented Feb 15, 2018

splitbrain commented Feb 15, 2018

lpaulsen93 commented Feb 15, 2018