Commit graph

10 commits

Author SHA1 Message Date
Bartosz Dziewoński ff0386239f Only detect comments with real signatures
Consequences of this are visible in the test cases:

* (en) Tech News posts are not detected.
  Examples: "21:22, 1 July 2019 (UTC)", "21:42, 29 July 2019 (UTC)"

* (en) Comments by users who customize the timestamp are not detected.
  Examples: "10:49, 28 June 2019 (UTC)", "21:34, 14 July 2019 (UTC)"

* (en) Comments with signatures missing a username are not detected.
  This sometimes happens if a comment is accidentally signed with
  '~~~~~' (five tildes), which only inserts the timestamp.
  Examples: "17:17, 27 July 2019 (UTC)", "10:25, 29 July 2019 (UTC)"

* (pl) A lone timestamp at the beginning of a thread is not detected.
  It's not part of a post, it was added to aid automatic archiving.
  Example: "21:03, 18 paź 2018 (CET)"

Bug: T245692
Change-Id: I0767bb239a1800f2e538917b5995fc4f0fa4d043
2020-02-21 01:30:54 +01:00
Bartosz Dziewoński 890588f36a Pick reply insertion point based on parser tree, not DOM tree
I don't like that I had to special-case `<p>` tags (top-level
comments) in this code. I feel like it should be possible to handle
top-level comments and replies in a generic way, but I couldn't find
a way to do it that actually worked.

Notes about changes to the behavior, based on the test cases:

* Given a top-level comment A, if there was a "list gap" in the
  replies to it: previously new replies would be incorrectly added at
  the location of the gap; now they are added after the last reply.
  (T242822)

  Example: "pl", comment at "08:23, 29 wrz 2018 (CEST)"

* Given a top-level comment A and a reply to it B that skips an
  indentation level: previously new replies to A would be added with
  the same indentation level as B; now they are added with the
  indentation level of A plus one. (The old behavior wasn't a bug, and
  this is an accidental effect of other changes, but it seems okay.)

  Example: "pl", comment at "03:22, 30 wrz 2018 (CEST)"
    and reply at "09:43, 30 wrz 2018 (CEST)"

* Given a top-level comment A, a reply to it B, and a following
  top-level comment C that starts at the same indentation level as B:
  previously new replies to A would be incorrectly added in the middle
  of the comment C, due to the DOM list structure; now they are added
  before C. (T241391)

  (It seems that comment C was supposed to be a multi-line reply that
  was wrongly indented. Unfortunately we have no way to distinguish
  this case from a top-level multi-line comment that just happens to
  start with a bullet list.)

  Example: "pl", comments at "03:36, 24 paź 2018 (CEST)",
    "08:35, 24 paź 2018 (CEST)", "17:14, 24 paź 2018 (CEST)"

* In the "en" example, there are some other changes where funnily
  nested tags result in slightly different results with the new code.
  They don't look important.

* In rare cases, we must split an existing list to add a reply in the
  right place. (Basically add `</ul>` before the reply and `<ul>`
  after, but it's a bit awkward in DOM terms.)

  Example: split-list.html, comment "aaa"; also split-list2.html
    (which is the result of saving the previous reply), comment "aaa"

* The modifier can no longer generate DOM that is invalid HTML, fixing
  a FIXME in modifier.test.js (or at least, it doesn't happen in these
  test cases any more).

Bug: T241391
Bug: T242822
Change-Id: I2a70db01e9a8916c5636bc59ea8490166966d5ec
2020-01-23 21:13:12 +01:00
Bartosz Dziewoński 30fcfec1fd parser: Merge multiple comments on one line
Even when you have multiple signatures by multiple users in one
paragraph (or list item), it's still basically a single comment.
We don't want to offer multiple buttons to reply to it.

The changed parser test cases are illustrative:
* All affected comments in the "pl" example are comments with a
  "post-scriptum", which is now more intuitively treated as part of
  the main comment.
* The first comment in the "en" example would probably have been
  better if it wasn't merged, but a weird use of the outdent template
  causes us to not be able to distinguish that the two parts of the
  comment display on separate lines.
* The last comment in the "en" example (isn't that neat?) was previously
  incorrectly treated as two comments, because there's a timestamp in
  the middle of it (the user is referring to another comment).
* Remaining affected comments in the "en" example are also comments
  with a "post-scriptum" and their treatment is clearly better now.

It also accidentally fixes some problems with modifier tests (but not
all), where previously <dl> nodes would be inserted in the middle of
<p> nodes, to reply to the comments which are now merged.

Bug: T240640
Change-Id: I0f2d9238aff75d78286250affd323cd145661a11
2020-01-22 02:21:43 +01:00
Bartosz Dziewoński da732843f3 Integration tests for the modifier
Document the current behavior of the modifier (which inserts the
replies into the DOM tree), so that we can more easily see the effect
of changes in I2a70db01e9a8916c5636bc59ea8490166966d5ec.

Basically, add a reply to every comment, and dump the resulting HTML,
comparing it to previously generated expected HTML (which can be
checked visually). Have a look at the new HTML files.

Notably, the very first section in the "pl" example demonstrates a
case of wrong reply location due to list gap :) (T242822).

Change-Id: I4aed0f0b112f53d98e3fe1da4d40db8687c7e537
2020-01-22 00:58:06 +01:00
Bartosz Dziewoński da668b72d5 Identify comments by username+timestamp+seq
Possible use cases:
* Matching comments between PHP and Parsoid HTML [implemented here]
* Finding the same comment in a different revision of a page
  (e.g. while resolving an edit conflict, or to allow resuming
  composition of autosaved comments) [implemented for highlighting
  user's own posted comment only]
* Permanent links to comments [future]

The reasoning for this form of ID is:
* _Timestamp_ by itself is a nearly unique identifier, so it's a good
  thing to start with
* Users may post multiple comments in one edit (or in many edits in
  one minute), so we need the _sequential number_ to distinguish them
* _Username_ is probably not required, but it may reduce the need
  for sequential numbers, and will help with human-readability if we
  add permanent links

The ID remains stable when a new comment is added anywhere by anyone
(excepts comments within the same minute by the same user), or when a
section is renamed.

It's not always stable when a comment is moved or when an entire
section is moved or deleted (archived), but you can't have everything.

Change-Id: Idaae6427d659d12b82e37f1791bd03833632c7c0
2019-12-09 13:45:31 +00:00
Bartosz Dziewoński 8b86d1abbc Document how to update the test JSON files
(And rebuild them using this method, the properties are in different
order, it's actually nice for readability to have 'replies' last)

Change-Id: Ib586e1081fa36cb9125db1b0b1d41f092350641c
2019-12-03 21:07:16 +01:00
Bartosz Dziewoński fc34556b04 Fix parsing links to subpages in user signatures
Change-Id: I381087c252eeb7530e63c4d99cecc1b2ee041b0a
2019-10-30 00:13:56 +01:00
Bartosz Dziewoński c83201b10c Fix parsing non-standard case in links to user contribs
Change-Id: I2da72e2731019ad5be0ba33aa229ad914a7aaf10
2019-10-30 00:13:56 +01:00
Bartosz Dziewoński e8012b7094 Fix incorrectly detecting a section heading inside the table of contents
Change-Id: I7209b523c3322b3b379e6aa82a4b2014cc39c404
2019-10-30 00:13:56 +01:00
Bartosz Dziewoński 112ea056cd Integration tests for parsing
Tests that handle a specific case and describe what they are testing
would be nice… but tests that just document the current status to
avoid regressions are also okay and easier to add.

Change-Id: I0b3530ae0e77de70932aaf623f5290d1876699a0
2019-10-30 00:13:56 +01:00