fix #8009 & #5969, symbol symbolSize and opacity setting for category itemStyle in graph #9171

Vvvickie · 2018-10-08T06:34:32Z

Ovilia

symbol and symbolSize is supported, but not symbolRotate, symbolKeepAspect, symbolOffset.
Very close now. Keep trying! 😃

Ovilia · 2018-10-10T02:48:37Z

src/chart/graph/categoryVisual.js

            var color = itemModel.get('itemStyle.color')
                || seriesModel.getColorFromPalette(name, paletteScope);
            categoriesData.setItemVisual(idx, 'color', color);
+            var opacity = itemModel.get('itemStyle.opacity')
+                || 1;


New line is no need here. ECharts project uses new line at 120 characters.

Ovilia · 2018-10-10T02:50:18Z

src/chart/graph/categoryVisual.js

+            categoriesData.setItemVisual(idx, 'opacity', opacity);
+            var itemSymbolType = itemModel.getShallow('symbol', true);
+            var itemSymbolSize = itemModel.getShallow('symbolSize',
+                true);


New line is no need here.

Ovilia · 2018-10-10T02:52:17Z

src/chart/graph/categoryVisual.js

+                categoriesData.setItemVisual(idx, 'symbol', itemSymbolType);
+            }
+            if (itemSymbolSize != null) {
+                // PENDING Transform symbolSize ?


Remove this comment.

src/chart/graph/categoryVisual.js

Ovilia · 2018-10-10T03:08:32Z

I added a test case for this issue: 9ebb31a
Please check test/graph-symbol.html to test it.

Vvvickie · 2018-10-10T08:39:05Z

Completed, please check it again : )

Ovilia · 2018-10-11T05:35:10Z

src/chart/graph/categoryVisual.js

@@ -88,6 +100,27 @@ export default function (ecModel) {
                        );
                    }

+                    if (!data.getItemVisual(idx, 'symbolKeepAspect', true)) {


Maybe a for-loop here?

Ummm, loop for what…？

['symbol', 'symbolSize', ''symbolKeepAspect', ...], all similar logic.

Already done :-)

Ovilia · 2018-10-11T08:13:27Z

src/chart/graph/categoryVisual.js

-            if (itemSymbolSize != null) {
-                categoriesData.setItemVisual(idx, 'symbolSize', itemSymbolSize);
-            }
+            var symbolStyleList = ['symbol','symbolSize','symbolKeepAspect','symbolOffset','symbolRotate']


Please mention your code style. Space after , and ; at the end.

Ovilia · 2018-10-11T08:14:02Z

src/chart/graph/categoryVisual.js

-            var itemSymbolOffset = itemModel.getShallow('symbolOffset',true);
-            if (itemSymbolOffset != null) {
-                categoriesData.setItemVisual(idx, 'symbolOffset', itemSymbolOffset);
+            for(var i = 0;i < symbolStyleList.length;i++){


Space after ;, for, and ). Please also check other places.

Ovilia · 2018-10-11T08:16:06Z

src/chart/graph/categoryVisual.js

-            if (itemSymbolOffset != null) {
-                categoriesData.setItemVisual(idx, 'symbolOffset', itemSymbolOffset);
+            for(var i = 0;i < symbolStyleList.length;i++){
+                var symbolStyleItem = itemModel.getShallow(symbolStyleList[i], true);


Did you test graph-symbol.html? It seems itemModel.getShallow('symbolOffset', true) always return undefined, so that symbolOffset: ['30%', 0] is config is not used. This doesn't seem right.

Both symbolOffset and other items return undefined, but somehow it works. I have checked that it can be consoled during the upper section.

for (var i = 0; i < symbolStyleList.length; i++) { var symbolStyleItem = itemModel.getShallow(symbolStyleList[i], true); if (symbolStyleItem != null) { categoriesData.setItemVisual(idx, symbolStyleList[i], symbolStyleItem); } }

In my local server, changing the symbolOffset do bring some effects to the symbol. You could put the percentage larger to see that.
But I am not sure whether the position is right since it is not computed by my code...
Really really sorry for those mistakes I have made...
Here is a new version with formatted code.

Reduct the latter symbolStyleList with symbolOffset and symbolRotate, it also works. Others need both two functions to do that.
In fact I don't know why...

Well, we have to make sure we understand our changes. 🤣
I'll check it later. Thanks!

Ovilia · 2018-10-17T04:53:04Z

@100pah please verify this PR.

100pah · 2018-10-23T11:04:30Z

It is a little complicated about this issue.
First of all, there is a "programming contract" about when and how to use the properies in option to render a chart in echarts:

Tow cases:

(A)
Basically, properties are retrieved from model (essentially, from "option" in model) directly when rendering in xxxView.
The model cascade mechanism ensure that get the right property.
The "cascade" means, for example:

itemModel - seriesModel (by default)
itemModel - categoryModel - seriesModel (in graph)
itemModel - levelModel - seriesModel (in tree, treemap, ...）
... (other cases)

(B)
If a properties is available to be modified by other components (e.g., legend, visualMap),
it will be retrieved in the "visual encoding stage", and be saved to data
(by data.setVisual() / data.setItemVisual()). When chart is finally being rendered in xxxView, it will
be retrieved from data (by data.getItemVisual()).

The "contract" is a little complicated and a little fuzzy. Why not just
trace them all via data.set/getItemVisual? I think there is two reasons:
(1) A chart needs many properties. If we retrieve them all and save to data and finally retrieve from data and
then render, it brings some redundancy of logic and code.
(2) Save obj by the unit of data item probably cause performance issue (mainly, GC issue) in large data.

In this case, we have these properties to be considered:
color, opacity, symbolSize, symbol, symbolOffset, symbolRotate, symbolKeepAspect.

Probably we have to trade them in different way:

(1) Follow to the contract (B) above:
color (implemented)
opacity (not implemented yet, can be implemented in categoryVisual like this PR did.）
symbolSize (not implemented yet, can be implemented in categoryVisual like this PR did.)

(2) Follow to the contract (A) above:
symbolOffset (implemented)
symbolRotate (implemented)
symbol (not implemented yet)

(3)
symbolKeepAspect (?)
I am not sure about this property. Currently it has been implemented follow the (A), but
probably it should not be able to modified by other components I think. What your opinion on it ?
@Ovilia

Vvvickie · 2018-10-31T14:53:35Z

I have submited a new one, but still a little confused about the relationship between model and data.
Please review the new changes.
@100pah I happen to know you are online...

src/chart/graph/categoryVisual.js

100pah · 2018-10-31T18:43:58Z

src/chart/graph/categoryVisual.js

        });

        // Assign category color to visual
        if (categoriesData.count()) {
            data.each(function (idx) {
+                var itemModel = categoriesData.getItemModel(idx);
+                var symbol = itemModel.getShallow('symbol', true);
+                if (symbol) {


I prefer do not support customizing symbol in this case.
Because it's a little complicated.
Set symbol to "data item visual" does not work.

Actually it is what the issue for...
So I just leave it there...? o.o

My fault. setItemVisual('symbol', ...) works...

src/chart/graph/categoryVisual.js

100pah · 2018-10-31T18:52:57Z

src/chart/graph/categoryVisual.js

+                                idx, itemStyleList[i],
+                                categoriesData.getItemVisual(category, itemStyleList[i])
+                            );
+                        }
                    }


After we have done all of this, there is another work need to do:

properties like color, opacity, symbolSize have been saved to data by data.setItemVisual,
but only color is retrieved from data (by data.getItemVisual(...)) and be used to render in GraphView.js.
We need to make opacity and symbolSize follow the same way, and then the entire patch can work.

Ummm, I am confused about this. Where is color retrieved?
The opacity and symbolSize also work currently...What's wrong? O.O

color, opacity, symbolSize are retrieved in src/chart/helper/Symbol.js (search 'getItemVisual' in this file). The src/chart/helper/Symbol.js, used by GraphView.js, is a common module abstracting the drawing and other behaviors of each node of the graph.

100pah · 2018-10-31T18:57:50Z

little confused about the relationship between model and data

data is an instance of echarts/src/data/List, model is an instance of echarts/src/model/Model, or, specifically, echarts/src/model/Series in this case.

Both of them contains user settings in echarts option, for example:

let option = {
    series: {
        name: 'asdf',
        type: 'line',
        itemStyle: { ... },
        data: [ ... ]
    }
};

The data properties is saved into a data instance, and the rest of the definitions of series is saved into a series model. And data is a propertied mounted on a series (model).

let data = model.getData();

100pah · 2018-11-04T10:20:37Z

src/chart/graph/categoryVisual.js

-            var opacity = itemModel.get('itemStyle.opacity') || 1;
-            categoriesData.setItemVisual(idx, 'opacity', opacity);
+            var opacity = itemModel.get('opacity');
+            if (opacity != undefined) {


A tiny issue, in most cases, xxx != null is a little better than xxx != undefined, although they have the same meaning. Probably because undefined can be redefined in some environments but null can not.

Ovilia · 2018-11-12T05:43:12Z

@Vvvickie Thanks for your contribution! 🌟🌟🌟🌟🌟
Now you are one of the ECharts contributors. Since we joined Apache group, you need to assign ICLA for your first PR.
Please fill in the PDF and print it, then sign on it and send the scanned file to secretary@apache.org and oviliazhang@gmail.com.
This may be a little tricky, and sorry for the trouble. This is required for the first time your commit is merged in. If you refused, your commit will be backed off.

And are you interested in being an ECharts committer? This has more restrict rules than contributors. Committers are those who contribute more frequently and play more important roles on ECharts project.

After being a committer:

You can have write access to the ECharts code
You can mange issues (add tags, assign to someone, ...)
You can discuss about features with us committers more frequently with Email
You name and personal page link will be displayed at ECharts Website About page
See this for more information
If you are interested, here's what you can do:

Subscribe our mailing list by send an email to dev-subscribe@echarts.apache.org
When you want to discuss with us about something, send email to dev@echarts.apache.org
Help others by replying issues regularly
Make more useful pull requests (fix bugs or make enhancement in issues)
Here are some issues that may be easier to be fixed: https://github.com/apache/incubator-echarts/projects/3

And you will be invited to be a committer when we think your contribution and knowledge of ECharts project is enough. We are looking forward to this! 😃

Vvvickie · 2018-11-12T09:41:55Z

Finished :）It seems you forget to attach the pdf. Anyway I find it somewhere else.
With pleasure to join you^.^

fix categoryVisual symbol symbolSize opacity

c309055

Vvvickie changed the title ~~fix categoryVisual symbol symbolSize opacity~~ fix #8009 & #5969, symbol symbolSize and opacity setting for category itemStyle in graph Oct 8, 2018

Ovilia requested changes Oct 10, 2018

View reviewed changes

Ovilia added a commit that referenced this pull request Oct 10, 2018

test(graph): add test case for graph category symbol #9171

9ebb31a

fix symbolKeepAspect symbolRotate symbolOffset

4a8e61c

delete console

fa319d2

Ovilia requested changes Oct 11, 2018

View reviewed changes

do symbolStyle Configs in a loop

81e8caf

Ovilia reviewed Oct 11, 2018

View reviewed changes

Vvvickie added 3 commits October 11, 2018 12:29

format codeStyle

104005c

format codeStyle

d011ec2

reduct the latter symbolStyleList

360e0d5

use itemMode to get symbol

8767981

100pah reviewed Oct 31, 2018

View reviewed changes

review

0e390af

100pah mentioned this pull request Nov 4, 2018

关系图中怎么使用多维度数据进行视觉映射visualMap？ #9326

Open

100pah reviewed Nov 4, 2018

View reviewed changes

Vvvickie added 4 commits November 4, 2018 15:23

add symbol

d801e18

add symbol

9dccfd0

about symbol

886d4c5

add symbol

2c2d8ef

100pah merged commit 93f5aac into apache:master Nov 4, 2018

100pah mentioned this pull request Jan 1, 2019

【echarts3】graph关系图中,series[i]-graph.categories[i].symbol的属性不能生效 #7016

Closed

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade echarts from 4.3.0 to 4.5.0 dyna-dot/iClient-JavaScript-s#1

Open

This was referenced Mar 24, 2020

[Snyk] Upgrade echarts from 4.2.1 to 4.6.0 OpenTMI/opentmi-default-gui#32

Merged

[Snyk] Upgrade echarts from 4.1.0 to 4.7.0 DeviaVir/zenbot#2011

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #8009 & #5969, symbol symbolSize and opacity setting for category itemStyle in graph #9171

fix #8009 & #5969, symbol symbolSize and opacity setting for category itemStyle in graph #9171

Vvvickie commented Oct 8, 2018 •

edited

Loading

Ovilia left a comment

Ovilia Oct 10, 2018

Ovilia Oct 10, 2018

Ovilia Oct 10, 2018

Ovilia commented Oct 10, 2018

Vvvickie commented Oct 10, 2018 •

edited

Loading

Ovilia Oct 11, 2018

Vvvickie Oct 11, 2018

Ovilia Oct 11, 2018

Vvvickie Oct 11, 2018

Ovilia Oct 11, 2018

Ovilia Oct 11, 2018

Ovilia Oct 11, 2018

Vvvickie Oct 11, 2018 •

edited

Loading

Vvvickie Oct 11, 2018 •

edited

Loading

Ovilia Oct 11, 2018

Ovilia commented Oct 17, 2018

100pah commented Oct 23, 2018

Vvvickie commented Oct 31, 2018 •

edited

Loading

100pah Oct 31, 2018

Vvvickie Oct 31, 2018

100pah Nov 4, 2018

100pah Oct 31, 2018

Vvvickie Oct 31, 2018

100pah Nov 4, 2018 •

edited

Loading

100pah commented Oct 31, 2018 •

edited

Loading

100pah Nov 4, 2018

Ovilia commented Nov 12, 2018

Vvvickie commented Nov 12, 2018

fix #8009 & #5969, symbol symbolSize and opacity setting for category itemStyle in graph #9171

fix #8009 & #5969, symbol symbolSize and opacity setting for category itemStyle in graph #9171

Conversation

Vvvickie commented Oct 8, 2018 • edited Loading

Ovilia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ovilia commented Oct 10, 2018

Vvvickie commented Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vvvickie Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Vvvickie Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ovilia commented Oct 17, 2018

100pah commented Oct 23, 2018

Vvvickie commented Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

100pah Nov 4, 2018 • edited Loading

Choose a reason for hiding this comment

100pah commented Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Ovilia commented Nov 12, 2018

Vvvickie commented Nov 12, 2018

Vvvickie commented Oct 8, 2018 •

edited

Loading

Vvvickie commented Oct 10, 2018 •

edited

Loading

Vvvickie Oct 11, 2018 •

edited

Loading

Vvvickie Oct 11, 2018 •

edited

Loading

Vvvickie commented Oct 31, 2018 •

edited

Loading

100pah Nov 4, 2018 •

edited

Loading

100pah commented Oct 31, 2018 •

edited

Loading