Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Sep 27, 2023

Conversation

amieiro
Copy link
Member

@amieiro amieiro commented Sep 22, 2023

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
@amieiro amieiro requested a review from akirk September 25, 2023 15:28
@amieiro
Copy link
Member Author

amieiro commented Sep 27, 2023

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.

Copy link
Member

@akirk akirk left a comment

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!

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.

Edge case? Placeholders & glossary words.
2 participants