Commit graph

193 commits

Author SHA1 Message Date
jenkins-bot 31879880f9 Merge "Base TreeWalker implementation on PHPDOM" 2020-11-20 18:22:32 +00:00
Ed Sanders 763ce88021 Base TreeWalker implementation on PHPDOM
For consistency with other DOM implementations.

Change-Id: I20447d880ccd3b70b6694b36ea2f63dd0c42fa84
2020-11-19 23:45:42 +00:00
jenkins-bot 62bc99bdde Merge "Use Parsoid DOMCompat/DOMUtils in CommentFormatter" 2020-11-17 18:05:52 +00:00
Ed Sanders 32cd64ec6a Use Parsoid DOMCompat/DOMUtils in CommentFormatter
As CommentFormatter no longer needs HTMLFormatter, remove
the inheritance and make addReplyLinks a static method.

Testing locally this is marginally slower, going from 2.55s
to 2.9s for the CommentFormatterTest case.

Bug: T266317
Bug: T267973
Change-Id: If69749cae678a1647a138d782a32032189f55cec
2020-11-16 22:28:07 +00:00
Ed Sanders 7d349808c2 ImmutableRange: Guard against appending empty fragments
This triggers a PHP warning.

Change-Id: I5ccb204287d55b38fadcef8cc846400a277e8491
2020-11-16 19:22:26 +00:00
Ed Sanders b03165fcce Compare node positions using upward traversal
A TreeWalker ends up walking potentially every single subsequent
node in the document looking for a target node. Instead use upwards
traversal to find a common ancestor, then sibling traversal to
compare document order.

This makes calling cloneContents on every comment on a 300k talk page
significantly faster, going from >30s to 500ms locally.

Change-Id: I28a2b8c11d4098d9bc44d19b98e19ccc02273098
2020-11-16 19:22:10 +00:00
jenkins-bot 5a5b2e61f1 Merge "ImmutableRange: Avoid doing expensive TreeWalker computation twice" 2020-11-15 14:43:01 +00:00
jenkins-bot 5937226f5f Merge "ImmutableRange: Skip redundant calls to isFullyContainedNode()" 2020-11-15 14:42:50 +00:00
Thiemo Kreuz 8ffe0d55da Remove comments that literally repeat what the code says
Change-Id: Ib928cf61dc512fbbf39a3279789376d635a82c52
2020-11-11 09:31:59 +01:00
jenkins-bot 579cc20120 Merge "Move ID->title logic into HeadingItem" 2020-11-05 18:10:08 +00:00
jenkins-bot e378a9122b Merge "Don't detect comments within headings" 2020-11-05 16:56:02 +00:00
Ed Sanders 79c91c3cfc Move ID->title logic into HeadingItem
Change-Id: I03408d2404a99b5bc7795c1c4bf214d4b5fea1e0
2020-11-05 16:10:39 +00:00
Ed Sanders 47b31101b8 ApiDiscussionToolsEdit: Remove stray line from old code
Change-Id: Id9561482d6fc2c2241790e61df1db71a4d520554
2020-11-05 16:02:58 +00:00
Bartosz Dziewoński 203a4dcb42 Use 'id' attributes rather than heading text for edit autosummaries
Ideally the edit autosummary would be generated in the same
way as in the old wikitext editor: from the wikitext of the
heading. But on the JS side, we don't have access to the
wikitext, or to the PHP method that generates autosummaries.

This might seem crazy at first, but ultimately the point of
the autosummaries is to link to the section heading by its
'id' attribute, so it is perfectly reliable.

Doing it this way depends on $wgFragmentMode being set to
[ 'html5', 'legacy' ] or [ 'html5' ], otherwise the escaped IDs
are super garbled (particularly in non-Latin-alphabet languages)
and can't be unescaped reliably. Conveniently, we already
require that since 9ee0fd69f5.

Bug: T264561
Bug: T266725
Change-Id: I7d35098d672d0edb50d49e22de1686d5cc83b60e
2020-11-05 15:13:08 +00:00
Bartosz Dziewoński bed717d329 Move getHeadlineNodeAndOffset() to utils
Needed by I7d35098d672d0edb50d49e22de1686d5cc83b60e.

Change-Id: I44bf927213de570fe9de43e485e09cfae6778eef
2020-11-05 16:11:30 +01:00
Bartosz Dziewoński 986e83ee61 Fix getHeadlineNodeAndOffset() returning text nodes
The condition was wrong, it could return either an element child with
.mw-headline, or a non-element child.

Bug: T267284
Change-Id: I28cda22ee8c5fe4a3259621adddd647b31291703
2020-11-05 16:09:35 +01:00
Bartosz Dziewoński 1626242863 Don't detect comments within headings
Bug: T267068
Change-Id: Id134f15e086fd070801c4b1d836dbfbf9bf444ad
2020-11-04 21:57:33 +01:00
jenkins-bot ad715a2724 Merge "Move warnings stuff from CommentItem to ThreadItem" 2020-11-02 22:03:36 +00:00
Bartosz Dziewoński 885d05b57b Handle any errors, not just exceptions
Internal PHP errors (such as "Call to undefined method…") are not Exceptions.
Follow-up to e18a0f3dcd.

Bug: T267035
Change-Id: I3cbf2b6b0d1d8a97cdac9791ec4f7b2ec807c7e5
2020-11-02 21:51:16 +01:00
Bartosz Dziewoński 31f6d44bf6 Move warnings stuff from CommentItem to ThreadItem
After recent changes allowing ThreadItems to have IDs, they can now
also have warnings about duplicate IDs.

Bug: T267035
Change-Id: If3edfe34e6e29741e29fac8946a3c88badc4ab7f
2020-11-02 20:07:23 +00:00
Bartosz Dziewoński e18a0f3dcd Handle exceptions in the OutputPageBeforeHTML hook
Instead of breaking the whole page, just log it and add a HTML comment.

Bug: T267035
Change-Id: Icec26cddb9db4816640d06881ed683e76067d54f
2020-11-02 20:15:37 +01:00
Bartosz Dziewoński bc2e4ed170 Make the "Advanced" menu preference hidden
Follow-up to 2f28cfdf56.

Bug: T261539
Change-Id: I8837885064b669e56dfaef9d1315926e7875d0b0
2020-10-29 15:43:34 +00:00
libraryupgrader fb6706a606 build: Updating mediawiki/mediawiki-codesniffer to 32.0.0
The following sniffs are failing and were disabled:
* MediaWiki.Commenting.PropertyDocumentation.MissingDocumentationPrivate
* MediaWiki.Commenting.PropertyDocumentation.MissingDocumentationProtected
* MediaWiki.Commenting.PropertyDocumentation.MissingDocumentationPublic

Additional changes:
* Dropped .inc files from .phpcs.xml (T200956).

Change-Id: I340d6b573e9ae2a99085fb19a705fcf567b03f92
2020-10-29 10:53:01 +00:00
Ed Sanders 3aca622894 Treat headings like comments now they have IDs
Use the same logic for marking ranges in the document, and ensure
that the heading range does not include section edit links or
section numberings.

Change-Id: I782caafc34fee2a822b0a17b24dd6b9528202eca
2020-10-28 12:38:18 +00:00
jenkins-bot 333486fd85 Merge "Add preference to expand the "Advanced" menu when replying" 2020-10-27 11:42:41 +00:00
jenkins-bot 6792c0c9c9 Merge "Include 'false' results in 'transcludedfrom' API response" 2020-10-27 11:38:52 +00:00
Bartosz Dziewoński 6f2ada2bb4 ImmutableRange: Avoid doing expensive TreeWalker computation twice
If A follows B, then we can assume that B does not follow A.
Calling the function recursively computes that twice,
we can instead make some simple changes to "invert" the result.

Change-Id: I709aca7cb997dd2fe3980468a8c6bde6f366fb5b
2020-10-22 23:39:14 +02:00
Bartosz Dziewoński a29cecdf73 ImmutableRange: Skip redundant calls to isFullyContainedNode()
It's an expensive method, and we previously called it for
every child of the common ancestor, completely unnecessarily.

These changes follow from two observations:
* If there is a $firstPartiallyContainedChild, then the
  first fully contained child must follow it; similarly,
  if there is a $lastPartiallyContainedChild, then the
  last fully contained child must precede it.
* All nodes between the first and last fully contained
  children are also fully contained.

Maybe it can be made cleverer still, but it's a lot better.

Change-Id: I4e596c62274c2c0be115f0ddec42629115b430a4
2020-10-22 23:31:21 +02:00
Bartosz Dziewoński ba8434e2e0 Add legacy IDs as of wmf/1.36.0-wmf.14
Bug: T264478
Change-Id: I099e1068fedc25d671cd1245ac8b32941dca7232
2020-10-22 22:52:59 +02:00
Bartosz Dziewoński 7ad6328223 Include 'false' results in 'transcludedfrom' API response
Skipping them could result in incorrect handling when RESTBase HTML is
outdated.

When a result for a given comment is not found, display an error
instead of assuming it is not transcluded.

Bug: T262065
Change-Id: I14a7a0a25d5181b5c49bd5677f0c002dce5a3cb9
2020-10-22 22:25:35 +02:00
Bartosz Dziewoński 0ddc171c8a Add oldest timestamp in the thread to heading IDs
To avoid old threads re-appearing on popular pages when someone
uses a vague title (e.g. dozens of threads titled "question" on
[[Wikipedia:Help desk]]: https://w.wiki/fbN), include the oldest
timestamp in the thread (i.e. date the thread was started) in the
heading ID.

Bug: T264478
Change-Id: If918bfd5e025248923d1939bc86916697ead95a0
2020-10-22 02:19:21 +02:00
Bartosz Dziewoński b09bbfe668 Disambiguate comments by parent ID, rather than sequential numbers
Sequential numbers aren't great because they change when an earlier
comment is archived. Parent comment/heading IDs should change less
often.

This also makes much more sense for disambiguating subsections,
e.g. a dozen identical ===Votes=== sections for a dozen proposals.

Bug: T264478
Change-Id: I466454984fd919ebef35f2b37ddb5d86dc842996
2020-10-22 02:19:21 +02:00
Bartosz Dziewoński 3137d76f40 Connect sub-threads to their parent threads
Our threads now also contain all replies to their sub-threads.
This is similar to how sections work in MediaWiki, where the parent
section also contains the content of all the lower-level sections.
We're going to need this for notifications about replies in a thread.

Bug: T264478
Change-Id: I241fc58e2088a7555942824b0f184ed21e3a8b6f
2020-10-22 02:05:02 +02:00
Bartosz Dziewoński 9ee0fd69f5 Allow headings to have IDs
Previously, only comments could have IDs, because we only needed IDs
for replying. But we might also use them for notifications soon.

Bug: T264478
Change-Id: I1bcad02bf17ab54bc5028a959543c10f0430836b
2020-10-22 02:04:28 +02:00
Bartosz Dziewoński 6719d17364 Handle cached "legacy" IDs (and other JSON-serialized data)
The output of CommentFormatter::addReplyLinks() and consequently
ThreadItem::jsonSerialize() can end up in the HTTP cache (Varnish) on
Wikimedia wikis. We need to consider that when changing that code.

Introduce a concept of legacy ID (generated by the older algorithm
after it changes), add some placeholder code that will generate them
in the future, and update some code to find comments by either normal
or legacy IDs.

Add dire comments in a bunch of places (as if that ever helps).

Bug: T264478
Change-Id: I4368f366800ab21b8b184b09378037614fdecd33
2020-10-22 00:53:06 +02:00
Bartosz Dziewoński 3b8d63467e CommentParser: Remove confused comments about references and objects
"This modifies the original objects…" – I feel like this is obvious
now, but maybe it wasn't so obvious when this code was structured
differently before a2431fe006. Also,
it refers to a variable that doesn't exist.

"FIXME this will clone the reply…" – No, actually, it will not.
It would if replies were associative arrays, but they are objects,
and have always been, ever since the PHP parser was merged in
7b7a2cd69c. Maybe they were arrays
once in Roan's mind before he pushed that for review.

Change-Id: I1348e111699fdbde99cd1f9ef45d8f465f7391b0
2020-10-21 21:01:27 +02:00
Bartosz Dziewoński 1ad6389292 ImmutableRange: Optimize parent check in computePosition()
We can check whether a node is a child of another node directly,
without iterating over all its children.

Change-Id: I3a26df89365bf765348d96b477c983ec9c4e43fe
2020-10-20 11:14:00 +02:00
Bartosz Dziewoński 2f28cfdf56 Add preference to expand the "Advanced" menu when replying
* Add the preference
* Only display it when the reply tool is enabled
* Use it when opening the reply tool
* Save it when the menu is toggled from the reply tool interface

Bug: T261539
Change-Id: Icb8fa6b3f1e9a3644669f21b08f34ea8c175f2f9
2020-10-20 07:09:40 +02:00
Ed Sanders d0bcec6196 Enable DT server side via a cookie to preserve user enable hack
Bug: T265499
Change-Id: Ied330c633732651d1c4e136646afd676ceb570c7
2020-10-19 21:42:58 +01:00
Bartosz Dziewoński 88b5be11fd CommentParser: Avoid unnecessary reference in foreach()
This is not necessary, and never has been. This variable contains an
object and it's never assigned to.

Instead, the reference creates hard-to-debug bugs (I've just spent
an hour debugging one). When the variable name is reused later in
the same function as the loop variable of another foreach() loop
(such as in If918bfd5e0), the result is overwriting of the last entry
in $this->threadItems with the last entry from the other array.
I was questioning everything I know about variables until I noticed.

Change-Id: Ibb57f915b39dd4d6d2e744903f9ecadd67b1f52d
2020-10-15 17:58:06 +02:00
jenkins-bot a2cf9cc978 Merge "Correctly generate timezone abbreviations for parsing" 2020-10-15 15:24:14 +00:00
Bartosz Dziewoński a1dc3a4896 Correctly generate timezone abbreviations for parsing
Also, add tests covering this and the previous bug fixes in this code
(T259818, T261706).

Note that the test data added in tests/cases/ doesn't exactly match
the entire configuration of the wiki, only the parts we want to cover.
This is unlike the data in tests/data/, which was literally copied
from the relevant wikis, and which is used as input for other tests.

Bug: T265500
Change-Id: I29a59a5952f6dc9fb5910434bb6bcc9dcdaa01a9
2020-10-15 12:11:25 +00:00
Bartosz Dziewoński fe520ab175 ImmutableRange: Remove unused variable
The only use was removed in code review, but we missed this.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/596813/70..71/includes/ImmutableRange.php

Change-Id: Ia761a6a350711c32f0bd4c8af48c7c1a35a4afb4
2020-10-15 11:23:43 +00:00
Ed Sanders 64392af485 Add reply links on the server
Bug: T252555
Change-Id: I4e60fdbc098c1a74757d6e60fec6bcf8e5db37c1
2020-10-08 13:27:08 +01:00
Bartosz Dziewoński ed17f640b6 Ignore other empty-ish things at the beginning of comments
Follow-up to 432a959436.

Bug: T264116
Change-Id: I0685cafab70c7e9d22f504f1a1309c9a28d6f2e1
2020-09-30 23:42:47 +02:00
jenkins-bot 91e87eb09a Merge "Fix detecting username from the wrong links sometimes" 2020-09-30 18:18:02 +00:00
jenkins-bot f7c7ba3c44 Merge "Ignore empty paragraphs at the beginning of comments" 2020-09-30 18:16:36 +00:00
Bartosz Dziewoński 17b7a481a2 Fix detecting username from the wrong links sometimes
When a timestamp directly followed a `<div>…</div>` tag (or perhaps
some other wrapper containing lots of content), we would detect the
username from the earliest links in the wrapper (furthest from the
timestamp), rather than the latest links (closest to the timestamp).

Bug: T262573
Change-Id: Id16449a86a731b13dc79846bb30ecf6554e26f1d
2020-09-29 22:31:24 +02:00
Bartosz Dziewoński 432a959436 Ignore empty paragraphs at the beginning of comments
The wikitext parser outputs `<p><br></p>` for empty paragraphs, so we
need to ignore `<br>` tags when searching for an "interesting" node
that marks the beginning of a comment. Otherwise the empty paragraphs
mess up the detection of indentation levels.

Bug: T264116
Change-Id: I84a97ab577baa7336b78935ccdc48041ecfc231a
2020-09-29 22:22:35 +02:00
Ed Sanders 6b8312e610 Ignore HTML comments which are more than two lines from a reply
Bug: T264026
Change-Id: I989132d7599a7fa156dba46d87a9ed4b76322c0c
2020-09-29 11:30:03 +01:00