Skip to content

Conversation

@cpeel
Copy link
Member

@cpeel cpeel commented Feb 27, 2024

When users are comparing diffs without formatting they expect "skip empty diffs" to skip pages that only differ by formatting. This links the two together so that it behaves the way that expect. Task 2000.

Sandbox: https://www.pgdp.org/~cpeel/c.branch/navigate-without-formatting/

@cpeel cpeel self-assigned this Feb 27, 2024
echo $navigation_text;
}

// Load an associative array for which pages in the project have diffs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're trying to get better about this could we add

/**
  * @return array<string, array<string, mixed>> 
  * Return an associative array indexed by page image name, where each page has an array of attributes.
*/
function load_page_diff_array(Project $project, Round $L_round, Round $R_round, bool $include_no_format = false): array

while ([$this_val, $this_user, $is_empty_diff] = mysqli_fetch_row($res)) {
foreach ($diff_array as $this_val => $diff_record) {
$this_user = $diff_record["username"];
$is_diff = $format == "remove" ? $diff_record["is_diff_without_formatting"] : $diff_record["is_diff"];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted to be slightly cleverer you could push the ternary inside the index:
$is_diff == $diff_record[$format == "remove" ? "is_diff_without_formatting": "is_diff"];
or

$idx = $format == "remove" ? "is_diff_without_formatting": "is_diff";
$is_diff = $diff_record[$idx];

Copy link
Member

@srjfoo srjfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested between P3 and F1, but probably a better time to use it is between F1 and F2, to see if F1 has made incorrect proofreading changes.

Copy link
Collaborator

@70ray 70ray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested a few pages and works well.

When users are comparing diffs without formatting they expect "skip
empty diffs" to skip pages that only differ by formatting. This links
the two together so that it behaves the way that expect.
@cpeel cpeel force-pushed the navigate-without-formatting branch from 04e2468 to 6b24b2c Compare April 27, 2024 04:20
@cpeel
Copy link
Member Author

cpeel commented Apr 27, 2024

Just rebased this branch and updated the sandbox.

This PR is being held until until we iron out if it's going in or not.

@cpeel cpeel marked this pull request as draft February 5, 2025 15:22
@cpeel
Copy link
Member Author

cpeel commented Feb 5, 2025

We're still waiting on feedback from people and this probably needs to be rebased so I put it in Draft to indicate that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants