Skip to content

Filter the placeholders to show them with the glossary words#1696

Merged
amieiro merged 14 commits into
GlotPress:developfrom
amieiro:placeholder-glossary
Sep 27, 2023
Merged

Filter the placeholders to show them with the glossary words#1696
amieiro merged 14 commits into
GlotPress:developfrom
amieiro:placeholder-glossary

Conversation

@amieiro

@amieiro amieiro commented Sep 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Problem

Imagine we have the show word in the glossary. If we have a placeholder in an original string finishing with the s letter and, without space, we have the how word: %show, the current algorithm detects the glossary word and not the placeholder, showing on mouse over a popover with the glossary information, instead of detects the %s placeholder and the how word.

  • Current approach: %show -> % + show
  • Proposed approach: %show -> %s + how

Fixes #1400.

Solution

This solution updates the regex that split the original string into an array, to isolate the placeholders, avoiding mixing parts of them with other substrings.

The regex used is the same used to detect warnings with the placeholders.

Testing Instructions

This PR adds two tests in the tests/phpunit/testcases/test_template_helper_functions.php file:

  • test_map_glossary_entries_with_placeholders_glued_glossary_words
  • test_map_glossary_entries_with_placeholders_glued_glossary_words_in_the_plural_origin

@amieiro amieiro requested review from akirk and trymebytes September 22, 2023 14:27
@amieiro amieiro changed the title Filter the placeholder to show with the glossary words Filter the placeholders to show them with the glossary words Sep 22, 2023
akirk
akirk reviewed Sep 25, 2023
View reviewed changes
Comment thread gp-templates/helper-functions.php Outdated
amieiro and others added 2 commits September 25, 2023 17:17
@amieiro amieiro requested a review from akirk September 25, 2023 15:28
@amieiro

amieiro commented Sep 27, 2023

Copy link
Copy Markdown
Member Author

To test the overhead in load time that this new regex might add, I used a Bash script to test it:

Before, I have:

  • Increased the number of items in an anonymous page from 15 to 500, changing this value here.
  • Added a real glossary with 293 items.

This is the script (change your local URL to test it):

#!/bin/bash

# Define the URL of the local web page
URL="https://glotpress.test/glotpress/projects/local-wp/dev/gl/default/"

# Number of times to measure the load time
NUM_ITERATIONS=100

# Initialize a variable to store the total load time
TOTAL_TIME=0

# Loop through the specified number of iterations
for ((i=1; i<=$NUM_ITERATIONS; i++)); do
    # Use curl to fetch the URL and measure the time it took
    load_time=$(curl -s -o /dev/null -w "%{time_total}\n" "$URL")
    
    # Add the load time to the total
    TOTAL_TIME=$(echo "$TOTAL_TIME + $load_time" | bc)
    
    # Print the load time for this iteration
    echo "Load time for iteration $i: ${load_time} seconds"
done

# Calculate the mean load time in milliseconds
mean_time=$(echo "scale=3; ($TOTAL_TIME / $NUM_ITERATIONS) * 1000" | bc)

# Print the mean load time in milliseconds
echo "Mean load time: ${mean_time} milliseconds"

This is the output after 100 executions:

  • Before: Mean load time: 781.000 milliseconds
  • With this PR: Mean load time: 778.000 milliseconds

I have obtained similar outputs with another executions, so no overload is apparent.

akirk
akirk approved these changes Sep 27, 2023
View reviewed changes

@akirk akirk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for doing the performance tests, looks good if it performs at the same speed!

@amieiro amieiro merged commit 6ebd75f into GlotPress:develop Sep 27, 2023
This was referenced Sep 28, 2023
Merged
Closed
This was referenced Oct 26, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akirk akirk akirk approved these changes

@trymebytes trymebytes Awaiting requested review from trymebytes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Edge case? Placeholders & glossary words.

2 participants

@amieiro @akirk