Support pure RTL text in form field through CPVT_section [pdfium : main]
Lei Zhang (Gerrit)
Lei Zhang added 11 comments![]( "Open in Gerrit")
calculate x-coords with `CFX_BidiCHar` for RTL chars in `CPVT_Section`.typo - 'H' should be lowercase.
bool bContinuous = continuous;Avoid using Hungarian notation for variable names in new code. Use Google C++ style for new names. Here and in the rest of this CL.
bool bContinuous = continuous;Give this local variable, and possibly also the parameter on line 313, better names so readers can more easily figure out what they represent.
float GetCaretX() const;Style: methods above member variables.
uint16_t Word = 0;Some of the changes here look like it's just modernizing the existing code, and isn't strictly necessary in this CL. Maybe split this off into a separate cleanup CL, and prepend that to this CL?
~CPVT_Word();Style: dtor after ctors, so move this after line 22.
enum class CPVT_WordDirection {Maybe nest this in `CPVT_WordInfo`. As in, `CPVT_WordInfo::Direction`.
printf("Char: %d, ptNew.x: %f\n", (int)word.Word, ptNew.x);This looks like a leftover debugging statement. Please remove.
Please run `optipng` on these files to keep the file sizes small.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seung Hyun Jin (Gerrit)
Seung Hyun Jin added 10 comments![]( "Open in Gerrit")
calculate x-coords with `CFX_BidiCHar` for RTL chars in `CPVT_Section`.typo - 'H' should be lowercase.
Done
Give this local variable, and possibly also the parameter on line 313, better names so readers can more easily figure out what they represent.
Done
Avoid using Hungarian notation for variable names in new code. Use Google C++ style for new names. Here and in the rest of this CL.
Done
Style: methods above member variables.
Done
Some of the changes here look like it's just modernizing the existing code, and isn't strictly necessary in this CL. Maybe split this off into a separate cleanup CL, and prepend that to this CL?
Done
Style: dtor after ctors, so move this after line 22.
Done
Maybe nest this in `CPVT_WordInfo`. As in, `CPVT_WordInfo::Direction`.
Done
printf("Char: %d, ptNew.x: %f\n", (int)word.Word, ptNew.x);This looks like a leftover debugging statement. Please remove.
Done
Please run `optipng` on these files to keep the file sizes small.
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
- Seung Hyun Jin
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 9 comments![]( "Open in Gerrit")
bool use_continuous_formatting,For historical reasons, PDFium started life without any code comments. Since you've been thinking about this, please document what this parameter means.
/*continuous=*/true, /*sub_word=*/0);Needs updating since the parameter name changed.
CFX_BidiChar::Direction eCurrentDirection = bidi.OverallDirection();1) Variable name style.
2) Since it's only used on line 738 to check for a particular direction, just precompute that here instead of once per loop iteration.
CPVT_WordInfo* pWord = word_array_[w].get();style
enum class CPVT_WordDirection { kLeftToRight = 1, kRightToLeft = -1 };As mentioned before, can shorten to "Direction".
enum class CPVT_WordDirection { kLeftToRight = 1, kRightToLeft = -1 };Do the values matter? i.e. Can these just be 0 and 1? Or would having `bool is_ltr` / `bool is_rtl` sufficient?
bool is_ltr = true;Would it be more consistent if this CL always used `is_rtl` or `is_ltr`? I feel I have to always be alert to the actual boolean. See the other comment in `CPVT_WordInfo`, but if `CPVT_WordInfo` stores a bool, then I would suggest consistently using that indicator everywhere else in this CL.
if (pIterator->GetWord(word)) {Since line 656 now calls GetWord(), is it necessary to call it again here? Same question for line 687 and others below.
if (ptNew.x != ptOld.x || ptNew.y != ptOld.y) {Can probably just compare directly: `ptNew != ptOld`. I guess this is really a comment about line 711.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seung Hyun Jin (Gerrit)
Seung Hyun Jin added 9 comments![]( "Open in Gerrit")
For historical reasons, PDFium started life without any code comments. Since you've been thinking about this, please document what this parameter means.
Done
Needs updating since the parameter name changed.
Done
CFX_BidiChar::Direction eCurrentDirection = bidi.OverallDirection();1) Variable name style.
2) Since it's only used on line 738 to check for a particular direction, just precompute that here instead of once per loop iteration.
Done
CPVT_WordInfo* pWord = word_array_[w].get();Seung Hyun Jinstyle
Done
enum class CPVT_WordDirection { kLeftToRight = 1, kRightToLeft = -1 };Do the values matter? i.e. Can these just be 0 and 1? Or would having `bool is_ltr` / `bool is_rtl` sufficient?
The values don't necessarily matter. Will change to bool is_ltr
enum class CPVT_WordDirection { kLeftToRight = 1, kRightToLeft = -1 };As mentioned before, can shorten to "Direction".
Done
Would it be more consistent if this CL always used `is_rtl` or `is_ltr`? I feel I have to always be alert to the actual boolean. See the other comment in `CPVT_WordInfo`, but if `CPVT_WordInfo` stores a bool, then I would suggest consistently using that indicator everywhere else in this CL.
changed all to is_rtl, an respectivly !is_rtl
Since line 656 now calls GetWord(), is it necessary to call it again here? Same question for line 687 and others below.
Done
Can probably just compare directly: `ptNew != ptOld`. I guess this is really a comment about line 711.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 7 comments![]( "Open in Gerrit")
word.set_is_rtl(pInfo->is_rtl);Considering `word` just got constructed, and this is the only set_is_rtl() caller, how about just add 1 more parameter to the CPVT_Word constructor instead?
float GetCaretX() const;I would recommend adding this as a straight-forward getter, without RTL support first, to reduce the size of this CL.
CPVT_WordInfo::CPVT_WordInfo()BTW, this is actually dead code. Just like CPVT_Word needed some cleanup, so does CPVT_WordInfo.
operator=(word);This pre-existing code implements a copy constructor in a strange way. In modern C++, one would just use `= default;` instead. Same for `operator=` below.
bool is_rtl = word.is_rtl();Since this variable is immediately used on the next line, and only used there, just inline it.
ptFoot.x = word.GetCaretX();Just reuse `ptHead.x` instead of calling GetCaretX() twice. This pattern repeats in several places.
float fX = pIterator->GetLineCaretX(line);Then match lines 1243-1245. No need for a local variable.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
CPVT_Word peek_word;Since there are no other CVPT_Word variables in this function, this can probably keep the "word" name and avoid the renaming churn.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seung Hyun Jin (Gerrit)
Seung Hyun Jin added 8 comments![]( "Open in Gerrit")
Since there are no other CVPT_Word variables in this function, this can probably keep the "word" name and avoid the renaming churn.
Done
Considering `word` just got constructed, and this is the only set_is_rtl() caller, how about just add 1 more parameter to the CPVT_Word constructor instead?
Done
I would recommend adding this as a straight-forward getter, without RTL support first, to reduce the size of this CL.
Done
BTW, this is actually dead code. Just like CPVT_Word needed some cleanup, so does CPVT_WordInfo.
Done
This pre-existing code implements a copy constructor in a strange way. In modern C++, one would just use `= default;` instead. Same for `operator=` below.
Done
Since this variable is immediately used on the next line, and only used there, just inline it.
Done
Just reuse `ptHead.x` instead of calling GetCaretX() twice. This pattern repeats in several places.
Done
Then match lines 1243-1245. No need for a local variable.
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
CPVT_WordInfo(const CPVT_WordInfo& word) = default;Where the "implementation" stays in the .cpp file.
CPVT_WordInfo::CPVT_WordInfo()Seung Hyun JinBTW, this is actually dead code. Just like CPVT_Word needed some cleanup, so does CPVT_WordInfo.
Done
Thanks for doing some cleanup here. Was hoping most of this can be prepended in a separate CL.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
float GetCaretX() const;I would recommend adding this as a straight-forward getter, without RTL support first, to reduce the size of this CL.
Lei ZhangDone
1. Was hoping this can be prepended in a different CL as well, regardless of the next bullet point.
2. Patchset 10 implemented GetCaretX() as: `return is_rtl_ ? location_.x : location_.x + width_;`, whereas patchset 12 implements it as `return location_.x + width_;`. Even though the implementation changed, all the tests still passed.
Does that mean:
a. Patchset 10 was doing unnecessary work?
or
b. There is not enough test coverage?
or
c. Something else?
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
int32_t font_index = word.font_index();I noticed patchset 11 made this change and a similar change in fpdfsdk/pwl/cpwl_edit_impl.cpp. I've been advocating for making this CL smaller and reducing the size of the diff, but this is going in the other direction and making the CL larger. So, how about also doing this in another CL? Can append to this CL since this change does not affect this CL too much either way.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seung Hyun Jin (Gerrit)
Seung Hyun Jin added 4 comments![]( "Open in Gerrit")
float GetCaretX() const;Seung Hyun JinI would recommend adding this as a straight-forward getter, without RTL support first, to reduce the size of this CL.
Lei ZhangDone
1. Was hoping this can be prepended in a different CL as well, regardless of the next bullet point.
2. Patchset 10 implemented GetCaretX() as: `return is_rtl_ ? location_.x : location_.x + width_;`, whereas patchset 12 implements it as `return location_.x + width_;`. Even though the implementation changed, all the tests still passed.
Does that mean:
a. Patchset 10 was doing unnecessary work?
or
b. There is not enough test coverage?
or
c. Something else?
There is not enough test coverage. My initial variabletext_unittest.cpp did not check for caret positioning.
Where the "implementation" stays in the .cpp file.
Done
CPVT_WordInfo::CPVT_WordInfo()BTW, this is actually dead code. Just like CPVT_Word needed some cleanup, so does CPVT_WordInfo.
Lei ZhangDone
Seung Hyun JinThanks for doing some cleanup here. Was hoping most of this can be prepended in a separate CL.
Done
I noticed patchset 11 made this change and a similar change in fpdfsdk/pwl/cpwl_edit_impl.cpp. I've been advocating for making this CL smaller and reducing the size of the diff, but this is going in the other direction and making the CL larger. So, how about also doing this in another CL? Can append to this CL since this change does not affect this CL too much either way.
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
EXPECT_EQ(word.CaretX(), word.location().x + word.width());1. This could have been in the previous CL, but that landed already...
2. Line 79 has `EXPECT_EQ(expected, actual);` so this should pass in the arguments in a consistent manner and not flip them.
3. It is difficult to see the test expectations are correct. If one expands out CaretX() without latest RTL changes, this expands out to `EXPECT_EQ(word.location().x + word.width(), word.location().x + word.width());`, which is not helpful because the 2 sides are essentially the same. It would be better to use the actual values.
float GetCaretX() const;Seung Hyun JinI would recommend adding this as a straight-forward getter, without RTL support first, to reduce the size of this CL.
Lei ZhangDone
Seung Hyun Jin1. Was hoping this can be prepended in a different CL as well, regardless of the next bullet point.
2. Patchset 10 implemented GetCaretX() as: `return is_rtl_ ? location_.x : location_.x + width_;`, whereas patchset 12 implements it as `return location_.x + width_;`. Even though the implementation changed, all the tests still passed.
Does that mean:
a. Patchset 10 was doing unnecessary work?
or
b. There is not enough test coverage?
or
c. Something else?
There is not enough test coverage. My initial variabletext_unittest.cpp did not check for caret positioning.
While it's good to have unit test coverage, it is still unclear how changing the CaretX() behavior affects the overall text rendering.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
Can this CL stand up on its own as-is and pass trybots? Either way, I'm concerned:
- Yes: If all these non-refactoring changes trigger little/no test failures, then that is a sign that the code has insufficient test coverage.
- No: Then the CL can't land as-is.
CPVT_Section. Update CPVT_Word::CaretX() and GetLineCaretX() toBut GetLineCaretX() didn't exist before this CL, so saying this CL is updating it sort of skipped a step.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Can this CL stand up on its own as-is and pass trybots? Either way, I'm concerned:
- Yes: If all these non-refactoring changes trigger little/no test failures, then that is a sign that the code has insufficient test coverage.
- No: Then the CL can't land as-is.
Red bots.
Related details
- Seung Hyun Jin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seung Hyun Jin (Gerrit)
Seung Hyun Jin added 4 comments![]( "Open in Gerrit")
Lei ZhangCan this CL stand up on its own as-is and pass trybots? Either way, I'm concerned:
- Yes: If all these non-refactoring changes trigger little/no test failures, then that is a sign that the code has insufficient test coverage.
- No: Then the CL can't land as-is.
Red bots.
Temporarily replaced expected images of Hebrew text
CPVT_Section. Update CPVT_Word::CaretX() and GetLineCaretX() toBut GetLineCaretX() didn't exist before this CL, so saying this CL is updating it sort of skipped a step.
GetLineCaretX() moved to caret calculation CL
EXPECT_EQ(word.CaretX(), word.location().x + word.width());1. This could have been in the previous CL, but that landed already...
2. Line 79 has `EXPECT_EQ(expected, actual);` so this should pass in the arguments in a consistent manner and not flip them.
3. It is difficult to see the test expectations are correct. If one expands out CaretX() without latest RTL changes, this expands out to `EXPECT_EQ(word.location().x + word.width(), word.location().x + word.width());`, which is not helpful because the 2 sides are essentially the same. It would be better to use the actual values.
Addressed in caret calculation CL
float GetCaretX() const;Seung Hyun JinI would recommend adding this as a straight-forward getter, without RTL support first, to reduce the size of this CL.
Lei ZhangDone
Seung Hyun Jin1. Was hoping this can be prepended in a different CL as well, regardless of the next bullet point.
2. Patchset 10 implemented GetCaretX() as: `return is_rtl_ ? location_.x : location_.x + width_;`, whereas patchset 12 implements it as `return location_.x + width_;`. Even though the implementation changed, all the tests still passed.
Does that mean:
a. Patchset 10 was doing unnecessary work?
or
b. There is not enough test coverage?
or
c. Something else?
Lei ZhangThere is not enough test coverage. My initial variabletext_unittest.cpp did not check for caret positioning.
While it's good to have unit test coverage, it is still unclear how changing the CaretX() behavior affects the overall text rendering.
It doesn't. Merely visual/aesthetic where caret does not move as expected (not moving accordingly). Appended CL as not part of this CL's scope
Related details
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |