The rendertype was not set so far, and unknown types were not
considered on filling the menu.
Also renaming from "Sun" to "Sun At Zenith", as just "Sun"
could be confused with the display of the sun in the space instead,
as done by the "stars" plugin.
Details
Details
- Reviewers
nienhueser rahn shentey - Group Reviewers
Marble
Diff Detail
Diff Detail
- Repository
- R34 Marble
- Branch
- addActionForSunAtZenithPlugin
- Lint
No Linters Available - Unit
No Unit Test Coverage
| src/plugins/render/sun/SunPlugin.cpp | ||
|---|---|---|
| 79 | A plugin that marks the location where the Sun is directly overhead at the zenith. | |
| src/plugins/render/sun/SunPlugin.cpp | ||
|---|---|---|
| 59 | Hm, for me "Solar Position" sounds like the position of the sun in the sky, less the point on the celestial body where the sun is directly overhead. So if I just see the name, without knowing what is behind I would expect something else or wonder what this would show and how at all. So would not prefer over "Sun At Zenith". | |
| 79 | This sounds better, yes, taking. | |
| src/plugins/render/sun/SunPlugin.cpp | ||
|---|---|---|
| 59 | Sun At Zenith Indicator? | |
Comment Actions
Seems things are slightly convoluted currently when it gets to under-the-sun things:
there is a separate dialog class SunControlWidget,, part of libmarble, which is explicitely added to the UI in marble-kde & marble-qt, activatable as menu entry "View">"Sun Control...". It allows to configure the sun-related display options for sun shading and the "subsolar point". The latter term is what we have been searching a name for so far in this review request.
The widget reads and sets the subsolar point related settings via methods of MarbleWidget, like setLockToSubSolarPoint/setSubSolarPointIconVisible.
MarbleWidget::setSubSolarPointIconVisible now iterates over all plugins to find the "sun" plugin and then toggles the visibility of that plugin.
Which in the end duplicates the action provided by the plugin.
So setting the RenderType of the "sun" plugin to ThemeRenderType would result in duplication in the menu, because then both the action of the plugin would be shown as well as the "Sun control" dialog action. So rather not an improvement, but the opposite.
To me it seems there needs to be some clean-up and untangling when it comes to plugins, to make Marble even greater:
- plugins should be self-contained and provide a complete feature on their own
- dependencies between plugins should be documented in the plugins metadata
- libmarble should not expose features in the API which rely on plugins, as those are optional by nature and might not be present.
Seems very related to what is noted in the email "Not existing map theme properties vs. existing plugins?", https://mail.kde.org/pipermail/marble-devel/2016-July/008762.html