Show more than first line in note list preview #1647
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmsnell
reviewed
Oct 16, 2019
|
Used the Dennis solution. |
|
Hold off on reviews. I need to fix the remove markdown functionality. |
|
I looked at how many times this actually gets called. It only gets called a dozen or so times for the items in the list that are visible. Let's get this fixed in. |
dmsnell
reviewed
Oct 18, 2019
dmsnell
reviewed
Oct 18, 2019
dmsnell
reviewed
Oct 18, 2019
dmsnell
reviewed
Oct 18, 2019
dmsnell
reviewed
Oct 18, 2019
dmsnell
reviewed
Oct 18, 2019
dmsnell
reviewed
Oct 18, 2019
dmsnell
force-pushed
the
fix/include-multiple-lines-in-excerpt
branch
from
4b84e2f to
89c68f8
Compare
dmsnell
added a commit
that referenced
this pull request
Oct 22, 2019
Replaces #1647 fixes: #235 The regex that captures the note list preview was only selecting the first line. This selects the remainder of the note. That string then gets sliced down to the `previewCharacterLimit`. This PR also decreases the performance cost of creating the title and preview from 1ms to 0.2ms. 1. Load production 2. Have note that has a few characters per line for a couple of lines 3. Have Settings -> Display -> Note Display set to Expanded 4. Observe only the first line of text is shown 5. Do the same on the this branch 6. Observe that multiple lines are shown up to 300 characters 7. This is the same as the Android app **Before:** <img width="331" alt="Screen Shot 2019-10-18 at 10 53 21 AM" src="https://user-images.githubusercontent.com/6817400/67104796-9690bd80-f195-11e9-959a-1edc18f58030.png"> **After:** <img width="329" alt="Screen Shot 2019-10-18 at 10 53 29 AM" src="https://user-images.githubusercontent.com/6817400/67104797-9690bd80-f195-11e9-9c17-282747d9a7da.png"> Only one developer ais required to review these changes, but anyone can perform the review. `RELEASE-NOTES.txt` was updated with: - Fixed bug that only shows the first line of text in note list preview
dmsnell
force-pushed
the
fix/include-multiple-lines-in-excerpt
branch
from
89c68f8 to
b3deb68
Compare
dmsnell
added a commit
that referenced
this pull request
Oct 27, 2019
Replaces #1647 fixes: #235 The regex that captures the note list preview was only selecting the first line. This selects the remainder of the note. That string then gets sliced down to the `previewCharacterLimit`. This PR also decreases the performance cost of creating the title and preview from 1ms to 0.2ms. 1. Load production 2. Have note that has a few characters per line for a couple of lines 3. Have Settings -> Display -> Note Display set to Expanded 4. Observe only the first line of text is shown 5. Do the same on the this branch 6. Observe that multiple lines are shown up to 300 characters 7. This is the same as the Android app **Before:** <img width="331" alt="Screen Shot 2019-10-18 at 10 53 21 AM" src="https://user-images.githubusercontent.com/6817400/67104796-9690bd80-f195-11e9-959a-1edc18f58030.png"> **After:** <img width="329" alt="Screen Shot 2019-10-18 at 10 53 29 AM" src="https://user-images.githubusercontent.com/6817400/67104797-9690bd80-f195-11e9-9c17-282747d9a7da.png"> Only one developer ais required to review these changes, but anyone can perform the review. `RELEASE-NOTES.txt` was updated with: - Fixed bug that only shows the first line of text in note list preview
dmsnell
force-pushed
the
fix/include-multiple-lines-in-excerpt
branch
from
b3deb68 to
f419154
Compare
Replaces #1647 fixes: #235 The regex that captures the note list preview was only selecting the first line. This selects the remainder of the note. That string then gets sliced down to the `previewCharacterLimit`. This PR also decreases the performance cost of creating the title and preview from 1ms to 0.2ms. 1. Load production 2. Have note that has a few characters per line for a couple of lines 3. Have Settings -> Display -> Note Display set to Expanded 4. Observe only the first line of text is shown 5. Do the same on the this branch 6. Observe that multiple lines are shown up to 300 characters 7. This is the same as the Android app **Before:** <img width="331" alt="Screen Shot 2019-10-18 at 10 53 21 AM" src="https://user-images.githubusercontent.com/6817400/67104796-9690bd80-f195-11e9-959a-1edc18f58030.png"> **After:** <img width="329" alt="Screen Shot 2019-10-18 at 10 53 29 AM" src="https://user-images.githubusercontent.com/6817400/67104797-9690bd80-f195-11e9-9c17-282747d9a7da.png"> Only one developer ais required to review these changes, but anyone can perform the review. `RELEASE-NOTES.txt` was updated with: - Fixed bug that only shows the first line of text in note list preview
dmsnell
force-pushed
the
fix/include-multiple-lines-in-excerpt
branch
from
f419154 to
192e3bf
Compare
dmsnell
reviewed
Oct 28, 2019
dmsnell
reviewed
Oct 29, 2019
dmsnell
reviewed
Oct 29, 2019
dmsnell
reviewed
Oct 29, 2019
dmsnell
reviewed
Oct 29, 2019
dmsnell
added a commit
that referenced
this pull request
Jan 16, 2020
Tests have been failing because `noteTitleAndPreview` is reported to be taking longer than 1ms to run. In fact, `Date.now()`, which we were using to validate this, includes reduced time precision for protection against timing attacks. In this patch I've done two things to address the failing tests: - Replace `Date.now()` with `process.hrtime()` which gives us nanosecond precision - Run the function-under-test 100 times in order to eliminate variance in the measurement While fixing this test I noted a flaw in `noteTitleAndPreview` that we missed in #1647. We created the helper function `removeMarkdownFromOutput` which transforms a string based on whether or not markdown is enabled for the note. Unfortunately for us we were setting a module-global function based on the module-init-time value `isMarkdown`, which as a function is always truthy, meaning that it would return a function that alwasy removed markdown regardless of the note. In this patch I've addressed this by passing `stripMarkdown` into that function. In the PR for #1647 we discussed this parameter and removed it but we kept `removeMarkdownFromOutput` outside the scope of `noteTitleAndPreview`. We could leave out the parameter and move the function into `noteTitleAndPreview` or we could add the parameter back. For the purpose of only creating that function once I have added the parameter back in. `noteTitleAndPreview` gets called in some inner loops and I preferred to avoid recreating the filtering function on every call, or at least until the JIT figured it out; I'm sure that the performance impact is insignificant but I don't see any benefit to one way over the other and I'd rather do less work as the default.
dmsnell
added a commit
that referenced
this pull request
Jan 16, 2020
Tests have been failing because `noteTitleAndPreview` is reported to be taking longer than 1ms to run. In fact, `Date.now()`, which we were using to validate this, includes reduced time precision for protection against timing attacks. In this patch I've done two things to address the failing tests: - Replace `Date.now()` with `process.hrtime()` which gives us nanosecond precision - Run the function-under-test 100 times in order to eliminate variance in the measurement While fixing this test I noted a flaw in `noteTitleAndPreview` that we missed in #1647. We created the helper function `removeMarkdownFromOutput` which transforms a string based on whether or not markdown is enabled for the note. Unfortunately for us we were setting a module-global function based on the module-init-time value `isMarkdown`, which as a function is always truthy, meaning that it would return a function that alwasy removed markdown regardless of the note. In this patch I've addressed this by passing `stripMarkdown` into that function. In the PR for #1647 we discussed this parameter and removed it but we kept `removeMarkdownFromOutput` outside the scope of `noteTitleAndPreview`. We could leave out the parameter and move the function into `noteTitleAndPreview` or we could add the parameter back. For the purpose of only creating that function once I have added the parameter back in. `noteTitleAndPreview` gets called in some inner loops and I preferred to avoid recreating the filtering function on every call, or at least until the JIT figured it out; I'm sure that the performance impact is insignificant but I don't see any benefit to one way over the other and I'd rather do less work as the default.
dmsnell
added a commit
that referenced
this pull request
Jan 17, 2020
Tests have been failing because `noteTitleAndPreview` is reported to be taking longer than 1ms to run. In fact, `Date.now()`, which we were using to validate this, includes reduced time precision for protection against timing attacks. In this patch I've done two things to address the failing tests: - Replace `Date.now()` with `process.hrtime()` which gives us nanosecond precision - Run the function-under-test 100 times in order to eliminate variance in the measurement While fixing this test I noted a flaw in `noteTitleAndPreview` that we missed in #1647. We created the helper function `removeMarkdownFromOutput` which transforms a string based on whether or not markdown is enabled for the note. Unfortunately for us we were setting a module-global function based on the module-init-time value `isMarkdown`, which as a function is always truthy, meaning that it would return a function that alwasy removed markdown regardless of the note. In this patch I've addressed this by passing `stripMarkdown` into that function. In the PR for #1647 we discussed this parameter and removed it but we kept `removeMarkdownFromOutput` outside the scope of `noteTitleAndPreview`. We could leave out the parameter and move the function into `noteTitleAndPreview` or we could add the parameter back. For the purpose of only creating that function once I have added the parameter back in. `noteTitleAndPreview` gets called in some inner loops and I preferred to avoid recreating the filtering function on every call, or at least until the JIT figured it out; I'm sure that the performance impact is insignificant but I don't see any benefit to one way over the other and I'd rather do less work as the default.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix
fixes: #235
The regex that captures the note list preview was only selecting the first line. This selects the remainder of the note. That string then gets sliced down to the
previewCharacterLimit.This PR also decreases the performance cost of creating the title and preview from 1ms to 0.2ms.
Test
Before:
After:
Review
Only one developer ais required to review these changes, but anyone can perform the review.
Release
RELEASE-NOTES.txtwas updated with: