feat(FillStyle): Add controls for fill color and opacity for shapes #693

kwilson-bits · 2019-07-19T21:49:50Z

Add controls for fill color and opacity for shapes, vector layers, and feature actions.
re #531

Testing link: http://os-fill-controls.surge.sh/

welchyd · 2019-07-23T21:28:15Z

I made a live link to help expedite testing: ~~http://late-garden.surge.sh~~ [outdated]
Test steps:

Draw a polygon (square, circle, or polygon) on the map and save it as a place
In the style accordion try out various Fill Style combinations for color and opacity
Verify that the polygon fill changes in the preview as different options are selected
Save the polygon and verify that it remembers the fill style when you edit it
In the layers menu expand the Saved Places layer and click on a place
Verify that the style accordion in the layers menu correctly shows the fill style
Verify that you can change the fill style
Verify that the style changes are toggleable with the command stack (ctrl + z and ctrl + y)
Verify that exporting Saved Places as a place preserves the fill style
Export saved places as a kml, verify that it imports with the fill style
Import and select the kml layer in the layers window and in the style accordion check 'replace feature style'
Verify that all shapes reflect the selected fill style
Export Saved Places as a csv, verity that it does not import with the fill style
Select the csv layer in the layers window and from the style accordion verify that all shapes reflect the selected fill style
In the layers window, right click on the csv file and from the menu select feature actions
Create a feature action that sets the style to have a fill
Verify that the fill changes as expected
Verify that exporting/importing the feature action with fill style works
Refresh Open Sphere and verify that the fill styles are persisted

I found a few small things:

Pre-existing, handle separately - When changing the fill color from the nodeui, the node doesn't update to match the new color

welchyd

featurestyleaction.js needs to be updated to include fill color and opacity for export/import of feature actions

src/os/ui/featureedit.js

src/os/command/feature/featurefillcolorcmd.js

src/os/command/feature/featurefillopacitycmd.js

src/os/command/feature/featureopacitycmd.js

src/os/command/feature/featurestrokecolorcmd.js

src/os/command/feature/featurestrokeopacitycmd.js

src/os/command/feature/featurefillopacitycmd.js

src/os/state/v4/baselayerstate.js

src/os/ui/file/kml/abstractkmlexporter.js

src/plugin/file/kml/kmlnodelayerui.js

src/os/ui/featureedit.js

src/plugin/file/kml/ui/kmlnode.js

jsalankey · 2019-07-26T17:14:20Z

Testing feedback:

Places are not visible while editing them due to the fact that the fill opacity slider is set to 0.
The fill style on points is overriding the stroke style. Changing the original stroke color picker has no effect if there's a fill color set.
Having a fill style slider at all with a point is confusing. This is part of a larger issue that also applies to the border style picker, so we may not be able to fix it here. However, they shouldn't conflict with each other.
The fill color is not reflected in the node icon in the layers window. This is a difficult problem also, to which the best solution is to have those icons show sort of both a stroke color on themselves and a fill color. We should write a followon to address this issue.
The fill color on a point place is applying and changing properly, but on refresh, it is not applying. The color is still set in the feature style UI on refresh, but it's not being applied.
Undoing a fill opacity change is always resetting the fill opacity to 100%.
The overall opacity slider now synchronizes both sliders. These two things should be able to be adjusted totally independently.

kwilson-bits · 2019-07-26T19:25:22Z

Testing feedback:

Places are not visible while editing them due to the fact that the fill opacity slider is set to 0.

The fill style on points is overriding the stroke style. Changing the original stroke color picker has no effect if there's a fill color set.

Having a fill style slider at all with a point is confusing. This is part of a larger issue that also applies to the border style picker, so we may not be able to fix it here. However, they shouldn't conflict with each other.

The fill color is not reflected in the node icon in the layers window. This is a difficult problem also, to which the best solution is to have those icons show sort of both a stroke color on themselves and a fill color. We should write a followon to address this issue.

The fill color on a point place is applying and changing properly, but on refresh, it is not applying. The color is still set in the feature style UI on refresh, but it's not being applied.

Undoing a fill opacity change is always resetting the fill opacity to 100%.

The overall opacity slider now synchronizes both sliders. These two things should be able to be adjusted totally independently.

The opacity slider behavior should be as follows:

When the stroke opacity and the fill opacity have the same value and the stroke slider is moved, the fill slider should move to match it. The idea being that if you have set the stroke and fill to be the same value, you want to keep them the same value.
As part of this, if you slide the stroke opacity to be the same as the fill opacity's value, and then keep sliding the stroke opacity, it will pick up the fill opacity and change them both to be the same.
Regardless of the stroke opacity value, when moving the fill slider you should only change the fill opacity. This way you can always change one without changing the other.

This should now behave the same for both commands and Feature Edit.

When undoing a fill opacity change, it should now properly reset to the previous value.

schmidtk

I would like to see the large volume of code duplication in this PR reduced.

src/os/command/feature/featurefillcolorcmd.js

src/os/command/vectorlayerfillcolorcmd.js

Stale

src/plugin/file/kml/kmlnodelayerui.js

src/os/ui/featureedit.js

schmidtk

Layers:

Layers that were loaded in the application prior to this change have a default fill opacity of 1 instead of 0. The layer should probably correct this in restore if os.style.StyleField.FILL_COLOR is not defined on the config.
Point geometries are using the fill color. That should only change config.fill, not config.image.fill. Points should be using the "Color" UI value.
If I undo the color, fill color, or fill opacity commands, the fill opacity is always set to 1.
The color Reset buttons (color and fill color) throw an error. More details in the code review.

Places:

The fill color command resets fill opacity to 1 on undo.
When the fill color matches the base color, changing the base color puts two commands on the stack. It should add a single command (likely multiple commands wrapped in a sequence command), and undo/redo should work for that command.
The fill color Reset button throws an error. More details in the code review.

Feature Actions:

Point geometries are using the fill color. That should only change config.fill, not config.image.fill. Points should be using the "Color" UI value.

src/os/ui/layer/vectorlayerui.js

src/plugin/file/kml/kmlnodelayerui.js

src/os/ui/layer/vectorlayerui.js

src/plugin/file/kml/kmlnodelayerui.js

src/os/command/feature/featurecolorcmd.js

src/os/command/vectorlayercolorcmd.js

src/os/ui/layer/vectorlayerui.js

src/plugin/featureaction/featurestyleaction.js

src/plugin/featureaction/ui/featurestyleactionconfig.js

src/plugin/file/kml/kmlnodelayerui.js

BREAKING CHANGE: Adds required argument to os.ui.file.kml.AbstractKMLExporter#createStyle

Fix an issue when undoing fill opacity. Fix when sliding stroke opacity when using Feature Edit dialog.

BREAKING CHANGE: This changes the behavior of `os.style.getConfigColor` when using a field hint. - If `config.<hint> === null`, return `null`. This allows disabling stroke/fill. - If `config.<hint>.color` is defined, return the value. - Return `undefined` otherwise, indicating the field/color could not be found.

schmidtk · 2019-08-19T12:34:23Z

I cleaned up remaining issues I've come across in testing, as well as a few bugs from master:

Undo feature color change will update the label color if it's the same as the feature.
Quick added places were not persisting their label columns, so labels would disappear after refresh.

Feedback resolved.

schmidtk · 2019-08-19T14:04:41Z

I deployed this to Surge at http://os-fill-controls.surge.sh/, updated in the issue description.

welchyd

When creating or editing a Place - the border dash options drop down doesn't show the initial selection

In the layers window, when the opacity and fill opacity sliders get set to the same value, changing the opacity slider also causes the fill opacity to change. This is not consistent with the behavior when creating/editing a Place

schmidtk · 2019-08-19T18:46:36Z

@welchyd

When creating or editing a Place - the border dash options drop down doesn't show the initial selection

This is broken on master, unrelated.

In the layers window, when the opacity and fill opacity sliders get set to the same value, changing the opacity slider also causes the fill opacity to change. This is not consistent with the behavior when creating/editing a Place

Ah thanks, missed that one when I was removing the opacity/fill opacity linkage. Fixed/deployed.

justin-bits · 2019-08-19T20:30:24Z

Opacity and Fill Style are not retained in saved states
Saved Places with default styling exported as KML, then imported as features are not visible on the map. Only the label is visible. Regression. If non default styling was applied before save, the places are visible. The features are also visible on mouseover, changing size or checking Replace Feature Style. The position for the Size slider definitely affects visibility. The features are not visible in the default position of 22.22%, but visible at any other position. On sliding left or right (visible), then returning to the default position, the features are again not visible.
Opacity is no longer respected on imported KML features unless Replace Feature Style is checked. Regression.

Follow-on issues here: #737

Changing layer opacity at the config level has significant performance implications, in that it creates new style objects for each opacity value. It also prevents the layer opacity control from affecting KML layers because the layer config is ignored.

Fill opacity is serialized in the fill color case.

schmidtk · 2019-08-20T16:09:11Z

@justin-bits I believe all of those are resolved, though I didn't see the second bullet prior to my latest changes.

This PR was making a fundamental change to how the layer-level opacity slider was working. Instead of using OL's get/setOpacity API's, it was changing the opacity in the layer style config by changing the color alpha values. This presents a few problems:

A new OL style will be created/cached for each opacity value.
Layer opacity no longer multiplied by feature opacity. This broke the layer opacity control for any layer with feature-specific styling (KML, layers with feature actions applied, etc). Fixing this would require computing the combined layer/feature opacity internally, which would incur a performance penalty.
Layer persist/restore behavior changed, impacting how state files are imported/exported.

To resolve these issues, the original layer opacity behavior was restored. This does mean the layer opacity will be multiplied by fill opacity, so if they're both at 50% the fill will be rendered at 25%. This seemed to be a reasonable compromise to avoid the above problems.

justin-bits · 2019-08-20T17:36:04Z

Looks good

welchyd suggested changes Jul 24, 2019

View reviewed changes

src/os/ui/featureedit.js Show resolved Hide resolved

jsalankey previously requested changes Jul 24, 2019

View reviewed changes

schmidtk reviewed Jul 24, 2019

View reviewed changes

src/os/command/feature/featurefillopacitycmd.js Outdated Show resolved Hide resolved

jsalankey reviewed Jul 24, 2019

View reviewed changes

src/os/state/v4/baselayerstate.js Outdated Show resolved Hide resolved

jsalankey reviewed Jul 24, 2019

View reviewed changes

src/os/ui/file/kml/abstractkmlexporter.js Outdated Show resolved Hide resolved

jsalankey reviewed Jul 24, 2019

View reviewed changes

src/plugin/file/kml/kmlnodelayerui.js Outdated Show resolved Hide resolved

welchyd previously requested changes Jul 26, 2019

View reviewed changes

src/os/ui/featureedit.js Outdated Show resolved Hide resolved

kwilson-bits force-pushed the 531-shape-fill-controls branch from 056714f to 325ad9b Compare July 26, 2019 15:27

jsalankey reviewed Jul 26, 2019

View reviewed changes

src/plugin/file/kml/ui/kmlnode.js Outdated Show resolved Hide resolved

schmidtk previously requested changes Jul 29, 2019

View reviewed changes

src/os/command/feature/featurefillcolorcmd.js Outdated Show resolved Hide resolved

src/os/command/vectorlayerfillcolorcmd.js Outdated Show resolved Hide resolved

kwilson-bits requested a review from jsalankey August 7, 2019 22:21

schmidtk suggested changes Aug 12, 2019

View reviewed changes

src/plugin/file/kml/kmlnodelayerui.js Outdated Show resolved Hide resolved

src/os/ui/featureedit.js Outdated Show resolved Hide resolved

kwilson-bits requested a review from welchyd August 12, 2019 16:56

schmidtk suggested changes Aug 13, 2019

View reviewed changes

schmidtk suggested changes Aug 15, 2019

View reviewed changes

kwilson-bits and others added 10 commits August 15, 2019 12:22

feat(fillstyle): add fill color/opacity controls

8d7a230

BREAKING CHANGE: Adds required argument to os.ui.file.kml.AbstractKMLExporter#createStyle

fix(fillstyle): fix how events are dispatched

7c75aa4

fix(fillstyle): fixes from code review

9108dbd

Fix an issue when undoing fill opacity. Fix when sliding stroke opacity when using Feature Edit dialog.

fix(fillstyle): lint fix

fb122c6

fix(fillstyle): consolidate multiple command files into fewer files

5700858

fix(fillstyle): hide the fill controls for points and lines

7e4f07c

fix(fillstyle): using existing functions for geometry checks

a819e1d

fix(kml): reuse existing event and fix plugin creep into os

bf5a9a7

test(feature): add tests for os.feature.getColor

cc8e20d

Kevin Schmidt added 2 commits August 19, 2019 06:18

fix(label): revert label color when it matches feature color

c2cade2

fix(places): persist labels when quick adding places

8a8c847

schmidtk self-requested a review August 19, 2019 12:44

schmidtk previously approved these changes Aug 19, 2019

View reviewed changes

Merge branch 'master' into 531-shape-fill-controls

0390ffd

welchyd suggested changes Aug 19, 2019

View reviewed changes

fix(places): unlink opacity/fill opacity sliders

7e32198

schmidtk dismissed their stale review via 7e32198 August 19, 2019 18:46

welchyd previously approved these changes Aug 19, 2019

View reviewed changes

fix(places): ignore opacity when comparing color/label color

d2bce3f

schmidtk dismissed welchyd’s stale review via d2bce3f August 19, 2019 19:58

jsalankey previously approved these changes Aug 20, 2019

View reviewed changes

schmidtk dismissed jsalankey’s stale review via 483da54 August 20, 2019 15:33

Kevin Schmidt added 2 commits August 20, 2019 09:40

fix(state): remove fill opacity case

60e997e

Fill opacity is serialized in the fill color case.

Merge branch 'master' into 531-shape-fill-controls

da62879

fix(style): use correct default image fill color

4905bf2

schmidtk requested a review from jsalankey August 20, 2019 20:00

jsalankey approved these changes Aug 23, 2019

View reviewed changes

schmidtk merged commit 3e75cd0 into ngageoint:master Aug 23, 2019

schmidtk deleted the 531-shape-fill-controls branch August 23, 2019 13:48

bradh mentioned this pull request Feb 24, 2021

Shape Fill Control #531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(FillStyle): Add controls for fill color and opacity for shapes #693

feat(FillStyle): Add controls for fill color and opacity for shapes #693

kwilson-bits commented Jul 19, 2019 •

edited by schmidtk

Loading

welchyd commented Jul 23, 2019 •

edited by schmidtk

Loading

welchyd left a comment

jsalankey commented Jul 26, 2019 •

edited

Loading

kwilson-bits commented Jul 26, 2019

schmidtk left a comment

schmidtk left a comment

schmidtk commented Aug 19, 2019

schmidtk commented Aug 19, 2019

welchyd left a comment •

edited

Loading

schmidtk commented Aug 19, 2019

justin-bits commented Aug 19, 2019 •

edited

Loading

schmidtk commented Aug 20, 2019

justin-bits commented Aug 20, 2019

feat(FillStyle): Add controls for fill color and opacity for shapes #693

feat(FillStyle): Add controls for fill color and opacity for shapes #693

Conversation

kwilson-bits commented Jul 19, 2019 • edited by schmidtk Loading

welchyd commented Jul 23, 2019 • edited by schmidtk Loading

welchyd left a comment

Choose a reason for hiding this comment

jsalankey commented Jul 26, 2019 • edited Loading

kwilson-bits commented Jul 26, 2019

schmidtk left a comment

Choose a reason for hiding this comment

schmidtk left a comment

Choose a reason for hiding this comment

schmidtk commented Aug 19, 2019

schmidtk commented Aug 19, 2019

welchyd left a comment • edited Loading

Choose a reason for hiding this comment

schmidtk commented Aug 19, 2019

justin-bits commented Aug 19, 2019 • edited Loading

schmidtk commented Aug 20, 2019

justin-bits commented Aug 20, 2019

kwilson-bits commented Jul 19, 2019 •

edited by schmidtk

Loading

welchyd commented Jul 23, 2019 •

edited by schmidtk

Loading

jsalankey commented Jul 26, 2019 •

edited

Loading

welchyd left a comment •

edited

Loading

justin-bits commented Aug 19, 2019 •

edited

Loading