NativeScript / nativescript-ui-charts

NativeScript wrapper around HiCharts library
Apache License 2.0
26 stars 6 forks source link

Critical: Version 0.1.0 of the plugin does not support disabling export chart button #17

Closed cjohn001 closed 3 years ago

cjohn001 commented 3 years ago

@shiv19 : Thank you very much for the update! Seems like we are getting close to stable now :) I am seeing two issues, most critical one in this report as it prevents me from using this version of the plugin:

Which platform(s) does your issue occur on?

Please, provide the following version numbers that your issue occurs with:

Please, tell us how to recreate the issue in as much detail as possible.

Normally, I should be able to dismiss the export button on a chart by one of the two below options

  1. in chart options with exporting disabled: exporting: { enabled: false },

  2. if exporting is enabled I should be able to disable the button as follows: exporting: { enabled: true }, navigation: { buttonOptions: { enabled: false } },

Both options unfortunately do not work with the new plugin version

shiv19 commented 3 years ago

@cjohn001 According to the exporting schema for iOS, the "enabled" property is not a boolean, but it is a number.

if you set it to 0 for false, and 1 for true, that should work.

you can either do that, or if you goto the exporting handler and change this to boolean in your node_modules, https://github.com/NativeScript/nativescript-ui-charts/blob/bab23680f98a9000cf2c8cfeac5aae230a2faf3b/src/options-handlers/exporting/exporting-handler.ts#L15

you can let me know if it works.

cjohn001 commented 3 years ago

@shiv19 : Thanks for your help. Alright, I double checked both options as my initial idea was also it might be 0 or 1 instead of true or false. Unfortunately, setting 0,1 does not work either. And according to the highcharts docs it should be boolean (same chart worked with v0.0.5 of this plugin). I changed the exporting handler from number to boolean data type. However, this unfortunately also does not have the desired effect. The export button is always shown. I also observed, that the export handler maps all boolean values to numbers. According to the highcharts api docs there are different other variables with boolean values (i.e. useMultiLevelHeaders) that also appear as number format in the export handler. See attached image.

Bildschirmfoto 2021-04-13 um 23 23 35

Would be great if you could have another look into it. Thanks a lot for your support. Let me know when I can support with testing.

shiv19 commented 3 years ago

But the image you shared is for the JavaScript API docs for HighCharts, https://github.com/NativeScript/nativescript-ui-charts/blob/bab23680f98a9000cf2c8cfeac5aae230a2faf3b/src/typings/objc!Highcharts.d.ts#L2594 If you look at this line in the typings, you can see that fallbackToExportServer is of "number" type in the iOS library. There is only 1 key that is boolean in that whole typings file for iOS (debug key). Which is why the options handler is mapping all booleans to number. On Android, all the keys you expect to be boolean, are actually boolean.

I'll take a look at the export menu stuff now.

cjohn001 commented 3 years ago

@shiv19 I now changed the format to a string enabled: 'string',

Then it is working. I am now unsure of the other booleans might be treated the same

shiv19 commented 3 years ago

Thanks for the update. After setting 'enabled' to a string, is it working on both android and ios?

cjohn001 commented 3 years ago

But the image you shared is for the JavaScript API docs for HighCharts, https://github.com/NativeScript/nativescript-ui-charts/blob/bab23680f98a9000cf2c8cfeac5aae230a2faf3b/src/typings/objc!Highcharts.d.ts#L2594

If you look at this line in the typings, you can see that fallbackToExportServer is of "number" type in the iOS library. There is only 1 key that is boolean in that whole typings file for iOS (debug key). Which is why the options handler is mapping all booleans to number. On Android, all the keys you expect to be boolean, are actually boolean. I'll take a look at the export menu stuff now.

I am now unsure how to interpret your answer. The chartoptions need to conform to the javascript API or is my asumption wrong? So far everything was working for me when using the javascript API docs

cjohn001 commented 3 years ago

Thanks for the update. After setting 'enabled' to a string, is it working on both android and ios?

I can only speak for IOS as I do not have my app running on Android yet

shiv19 commented 3 years ago

I'm letting you pass the chartOptions according to the javascript API docs, but internally, it has to conform to the schema provided by the native libraries for iOS and Android :)

shiv19 commented 3 years ago

Could not disable the export button on iOS. It is working as expected on Android. I tried a bunch of things so far. No dice. I'll revisit it after sometime. Setting it to string did not work for me.

cjohn001 commented 3 years ago

@shiv19: I spend some time to research further into the topic and at the same time did migrate the angular demo to the new plugin sources (see PR).

What I found out: I suspect the source of the issue is, that the IOS high charts library interface is not a number but a pointer to a NSNumber.

https://api.highcharts.com/ios/highcharts/Classes/HIExporting.html#//api/name/enabled @property (nonatomic, readwrite) NSNumber *enabled

Regarding my observation with setting the "enabled" attribute to a "string" type. This works now also with IOS and the updated Angular demo I provided in the PR. Note: In the demo I changed the chart options of the line chart to disable the export button, hence you can directly see if things are working or not. In order to make it work on IOS, you would need to switch the ios runtimes from "@nativescript/ios": "^8.0.0" -> "tns-ios": "6.5.4" and then change the type to string. However, this is not the right way to go forward. The new runtime and the android runtime both do not work with this approach. I assume the NSNumber Pointer type needs to be dealt with in a different way than with a number type.

So far I was developing with v0.0.5 of this plugin, where disabling of the export button worked well with the tns-ios runtime. Important: The 0.0.5 plugin did not work for me with the new ios runtime and android, and I now understand why. I did a diff between all files of v0.0.5 and v0.1.0.

You made a change in https://github.com/NativeScript/nativescript-ui-charts/blob/master/src/options-handlers/helpers/helpers.ios.ts

In version 0.0.5 it was:

export function fromJSToNativePrimitive(value) { return value; }

and in 0.1.0 it is: export function fromJSToNativePrimitive(value) { // stub if (typeof value === 'boolean') return value ? 1 : 0; return value; }

Hence false and true are converted to numbers. I understand that this should be in general correct what you did. However, if I undo this, things again work with the tns-ios runtime and the v8 runtimes break (hence back to old behavior).

I know that undoing it is not the right way. Seems like closing one bug opened the appearance of another bug. The ios highcharts library requires a number pointer. Unfortunately, I could not find out so far how the right value type would have to look like to pass it to the library. Would be great if you could solve this. Unfortunately, I have no ideas anymore what is going wrong here.

@NathanWalker : I have seen that you did quite some work on this plugin as well. Do you have an idea what the problem might be?

cjohn001 commented 3 years ago

Hello @shiv19, @NathanWalker , I am wondering if one of you had a chance to look into the issue. I have spend quite some time in order to get the issue fixed, but without success yet. I found some info to the setup of the relevant attribute in the following issues:

https://github.com/highcharts/highcharts-ios/issues/42 https://github.com/highcharts/highcharts-ios/issues/47

Therefore I tried to set the HiExporting object directly to

exporting.enabled = [[NSNumber alloc] initWithBool:false];

Unfortunately, this also does not help. As the plugin worked in the old version with the old ios runtime and the plugin is still using the same lib underneath, I in the meantime suspect that it is not an issue of the plugin but the new runtime in respect to how the NSNumber object is mapped to native code. Would be great if one of you guys could have a look into it. So far I have not found a way to set up Nativescript projects in order to debug them in XCode, hence everything happening in objective C code is currently a blackbox to me. If someone of you would have an advice how to do it, I would try to look into it again. But for this I would require some directions how to setup such a debug project.

cjohn001 commented 3 years ago

Hello Shiva @shiv19, I am wondering if you had another look into this issue. As I still have not found a solution, I am currently thinking about building a custom version of the highcharts library in which the export button is disabled by default. As this would really be a bad workaround and as I do not want to maintain a fork of this plugin, I am wondering if you did make progress on the issue. Thanks for your feedback!

Best regards, Christoph

shiv19 commented 3 years ago

Hi @cjohn001 I haven't been able to investigate this issue further.

I have attached a zip file of the version of highcharts that I use in the project I work on (extract the zip file and install the tgz file)

We are having no issue in hiding the export button on this plugin,

what's strange is that the code base on both plugins is pretty much the same, I couldn't figure out why you are not able to hide the export button in plugin of this repo.

We're just passing exporting as false in the chart options and it is hiding the button on both platforms for us. image

nativescript-highcharts-1.0.16-npm.tgz.zip

The import path for this plugin is @imakeltd/nativescript-highcharts everything else is the same

cjohn001 commented 3 years ago

@shiv19 thanks for the sources. I created a diff and think I know why you are not seeing the bug. Seems like your project is not on NS7 yet. Your package.json shows "@nativescript/core": "6.5.8". I have also seen that the generated js files you provided in the package look different then in my project. I assume your project is not using es2017 yet.

The bug definitely appears at least from NS7 onwards. With my last PR I updated the angular-demo to recent nativescript packages. Could you please run the angular demo and tell me if you are seeing the bug?

Just run
npm run build && npm run demo-angular.ios

In the demo-app please go to 'Basic Line Chart (async data)' If you are seeing the export button, see attached image, you are seeing the bug in action as I have set:

exporting: { enabled: false, },

in app/demos/line-charts/basic-line/basic-line.component.ts

Bildschirmfoto 2021-05-29 um 21 01 55

Thank you very much for your help!

cjohn001 commented 3 years ago

@shiv19 I will report a workaround for the issue here that we can at least use the plugin with the old JSC runtime, i.e. "@nativescript/ios": "6.5.4" till a real solution to the problem was found.

The old runtime works by replacing a function in helpers.ios.ts with

export function fromJSToNativePrimitive(value) { return value; // stub // if (typeof value === 'boolean') return value ? 1 : 0; // return value; }

Seems like the old runtime requires HIExporting.enabled to be set to true or false rather than 0,1 in order for the value to get marshalled to an NSNumber object of type boolean. With 0,1 I assume it get serialized to NSNumber of type double.

Unfortunately, with this workaround all newer versions of the Nativescript core runtime (v8 engine) break. The app then crashes. This is to say unfortunately we still do not have a solution! :(

@shiv19 : Would be great if you could have another look at it. I unfortunately do not know how I could debug things further. Maybe you have access to the highcharts-ios sources and you can see what the setOptions functions in objective C receives?

I am rather sure in the meantime, that HIExporting.enabled needs to be serialized to an NSNumber object of type boolean. Unfortunately, I do not have an idea how this can be enforced from Javascript. I tried by explicitly setting the value to NSNumber.alloc().initWithBool(false) but this also does not work. Seems like marshaling converts the object to something different which again breaks things.

shiv19 commented 3 years ago

The actual highcharts lib for android and ios are closed source. But since my company has a paid license for highcharts, I can try raising a support ticket with them to help debug this issue.

Just to reiterate, this issue only occurs on iOS, right? And only on the v8 iOS runtime.

cjohn001 commented 3 years ago

@shiv19: Thanks a lot. Yes this is right, only iOS and the v8 ios runtime. I assume, the problem is not with highcharts-ios, but with the nativescript v8 runtime. If your company has support from the nativescript team, I assume this would be the better place to ask for help. As mentioned, I assume the problem is that the highcharts-ios library requires a NSNumber object of type boolean and that the v8 nativescript runtime marshals to an NSNumber object of numeric type instead of boolean.

I asked regarding highcharts-ios code, because I assume with the code it would be possible somehow to debug what the objective c-code receives as a parameter from the nativescript runtime.

shiv19 commented 3 years ago

We have support from highcharts team actually.

But I'm a member of the nativescript core team too. So I can ask them as well.

cjohn001 commented 3 years ago

This is great!

cjohn001 commented 3 years ago

If there should arise questions with my description or about what is going wrong, I could provide more details via a telco. Just let me know.

shiv19 commented 3 years ago

I've sent a slack invite to your gmx.de email address :)