Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

TextElementsRenderer supports Font and TextStyle updates #2008

Merged
merged 7 commits into from
Dec 14, 2020

Conversation

FraukeF
Copy link
Contributor

@FraukeF FraukeF commented Dec 8, 2020

Thank you for contributing to harp.gl!

Before requesting a pull request, please remember to check the following documents:

If you are adding new functionality we would highly appreciate if you can describe what is the capability you are adding and even better if you can add some examples. Please also remember to add tests for it.

CI Check

Our bots will check whether your PR can be directly integrated into the mainline. We have some internal integration tests running on the background, our bots will inform you of the next steps and someone from our team will take a look and help if needed!

And please do not forget to sign-off your commit! You can read more about DCO here. But, in short, you just need to use git commit -s or append --signoff when you are committing to the repo.

Happy contributing!

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #2008 (27d46a1) into master (ef945ee) will increase coverage by 0.08%.
The diff coverage is 85.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2008      +/-   ##
==========================================
+ Coverage   66.22%   66.31%   +0.08%     
==========================================
  Files         294      294              
  Lines       26299    26314      +15     
  Branches     5909     5929      +20     
==========================================
+ Hits        17417    17449      +32     
+ Misses       8882     8865      -17     
Impacted Files Coverage Δ
@here/harp-mapview/lib/text/Placement.ts 87.33% <ø> (ø)
...rp-mapview/lib/text/TextElementsRendererOptions.ts 100.00% <ø> (ø)
@here/harp-mapview/lib/text/TextStyleCache.ts 55.06% <70.00%> (+0.18%) ⬆️
...here/harp-mapview/lib/text/TextElementsRenderer.ts 71.97% <85.27%> (+2.34%) ⬆️
@here/harp-mapview/lib/MapView.ts 73.27% <100.00%> (-0.38%) ⬇️
@here/harp-mapview/lib/MapViewThemeManager.ts 81.18% <100.00%> (ø)
@here/harp-mapview/lib/text/FontCatalogLoader.ts 100.00% <100.00%> (+20.00%) ⬆️
@here/harp-mapview/lib/poi/PoiRenderer.ts 43.22% <0.00%> (+2.19%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef945ee...27d46a1. Read the comment docs.

@FraukeF FraukeF marked this pull request as draft December 9, 2020 05:27
@FraukeF FraukeF force-pushed the textelementsrenderer-refactor branch 3 times, most recently from 17d2f8a to 8ae740d Compare December 9, 2020 06:57
@FraukeF
Copy link
Contributor Author

FraukeF commented Dec 9, 2020

@harpgl_bot retest this please

@FraukeF
Copy link
Contributor Author

FraukeF commented Dec 10, 2020

@harpgl-bot retest this please

@FraukeF
Copy link
Contributor Author

FraukeF commented Dec 10, 2020

@harpgl-bot retest this please

@FraukeF FraukeF force-pushed the textelementsrenderer-refactor branch 2 times, most recently from 0300129 to ec06a43 Compare December 10, 2020 08:15
@FraukeF FraukeF marked this pull request as ready for review December 10, 2020 10:02
@FraukeF
Copy link
Contributor Author

FraukeF commented Dec 10, 2020

@harpgl-bot retest this please

Copy link
Collaborator

@atomicsulfate atomicsulfate left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments.

Frauke Fritz added 3 commits December 11, 2020 10:43
   * FontCatalogLoader only loads FontCatalogs
   * MapView only ever has one instance of TextElementsRenderer
   * MapViewThemeManager awaits Theme updates on DataSources

Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
…nderer

Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
@FraukeF FraukeF force-pushed the textelementsrenderer-refactor branch 2 times, most recently from 36a2133 to 95f47d2 Compare December 11, 2020 12:07
Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
@FraukeF FraukeF force-pushed the textelementsrenderer-refactor branch from 95f47d2 to fb398c8 Compare December 11, 2020 12:13
Frauke Fritz added 2 commits December 11, 2020 15:32
Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
@FraukeF FraukeF force-pushed the textelementsrenderer-refactor branch from 313f1f5 to ca3ebae Compare December 14, 2020 11:30
Copy link
Collaborator

@atomicsulfate atomicsulfate left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments.

}
this.m_textStyleCache.initializeTextElementStyles(this.m_textCanvases);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed both here and in updateTextStyles? Because MapView calls updateFontCatalogs and updateTextStyles immediately after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently this is still needed, as in theory both methods can be called separately,..
But, I think in the next step in this process, which will make the TextStyleCache updatable instead of recreating a new one every time the theme is set, this will most probably disappear.

Signed-off-by: Frauke Fritz <frauke.fritz@here.com>
@FraukeF FraukeF merged commit 9c589d4 into master Dec 14, 2020
@FraukeF FraukeF deleted the textelementsrenderer-refactor branch December 14, 2020 14:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants