SAP / openui5

OpenUI5 lets you build enterprise-ready web applications, responsive to all devices, running on almost any browser of your choice.
http://openui5.org
Apache License 2.0
2.94k stars 1.23k forks source link

Documentation: `sap.m.p13n.Engine` and its helper classes documentation is inconsistent and unclear #3963

Closed iljapostnovs closed 6 months ago

iljapostnovs commented 7 months ago

OpenUI5 version: 1.120.0 TS Types: 1.120.0

There are a lot of inconsistencies from typescript and API perspective when using sap.m.p13n.Engine.

  1. Let's start from looking into samples from UI5: image

sap.m.p13n.Engine.getInstance doesn't exist in the documentation and types.

image image


MetadataHelper doesn't receive any parameters according to types and documentation

image image But it does in the code and the UI5 Sample: image


SelectionController and the rest of the controller constructor according to types and documentation should receive id first and only then the settings, however, the sample and actual library code is different

Sample code from UI5: image Constructor from TS: image Constructor from documentation: image Constructor in the library itself: image


Event handlers doesn't have proper types defined:

image


Long story short, all p13n Engine topic is unusable from TS perspective.


Additionally it is totally unclear how can I control the state of the Engine:

  1. How can I provide the list of already visible columns (or maybe already used sorters)? Doesn't seem that something can be passed to SelectionController, Engine instance doesn't seem to have anything for that as well, except applyState method which is experimental.
  2. What can be passed to Engine -> show method? image Where did "Columns" and "Sorter" come from? According to documentation they are the keys, but there is no information if those keys are something which is predefined, or maybe I should define them by myself. If they are predefined, what are the values? If they are not, where should I specify them?

Absence of answers on those questions makes usage of the Engine tricky.


My current task is to implement column visibility personalization for Grid Table and we are using version 1.108.*. There are 4 options to do it:

  1. Using standard showColumnVisibilityMenu for Grid Table, but it is deprecated since 1.117, so it makes no sense to use it.
  2. Using p13n Engine, but it is totally broken for TS
  3. Implementing totally custom Menu with reimplementation of standard menu items, which is recommended by documentation. Funny recommendation if you ask me :)
  4. Making totally custom p13n dialog.

4 options and all of them are poor. P13n Engine would be the best option, if it would have proper documentation and types.


It would be really nice if this could be fixed as a patch for 1.108.* as well, because I am forced to implement everything fully custom otherwise. Also it would be great if there would be some proper documentation and samples on how to use the Engine, current documentation state, as you can see, is chaotic and unclear.

Thanks in advance!

Todor-ads commented 7 months ago

Hello @iljapostnovs , Thank you for sharing this finding. I've created an internal incident DINC0056931. The status of the issue will be updated here in GitHub.

Regards, Todor

martinhaeuser commented 7 months ago

Hello @iljapostnovs,

thank you for the feedback on the personalization topic. We've recently published two fixes for the documentation to improve the generated type definitions.

I will prepare downports for the according documentation correction and corrseponding type definition generation for the earlier releases, too. I will provide an update soon.

As you are referring to the examples, have you also checked the developer guide which provides a step by step description on the integration and provides further explanations? You can find this guide just below the according object samples which answers the questions you've been asking: https://sapui5.hana.ondemand.com/#/topic/f28025135e0e4527bdfb7c5441647391

Regards Martin

iljapostnovs commented 7 months ago

Hello @iljapostnovs,

thank you for the feedback on the personalization topic. We've recently published two fixes for the documentation to improve the generated type definitions.

I will prepare downports for the according documentation correction and corrseponding type definition generation for the earlier releases, too. I will provide an update soon.

As you are referring to the examples, have you also checked the developer guide which provides a step by step description on the integration and provides further explanations? You can find this guide just below the according object samples which answers the questions you've been asking: https://sapui5.hana.ondemand.com/#/topic/f28025135e0e4527bdfb7c5441647391

Regards Martin

Thank you!

Regarding the guide, they don't really answer on my questions. E.g. 1) Question regarding handling the state. Yes, there are examples with e.g. applyState, however, as I mentioned before, it is experimental method, which means that I can't trust it. image So what can be used to apply state without experimental features? 2) Question regarding the keys image

"Desired keys" doesn't say a lot. Where do the keys come from? Are those the keys from register method -> controller field? Can I set those keys as any string value? I hope so, because I see no other logical connection. If it is true, it would be nice if that would be mentioned somewhere.

martinhaeuser commented 6 months ago

I wanted to inform you about the fixes performed on this issue. We provided several fixes: Improve documentation for MetadataHelper: https://github.com/SAP/openui5/commit/f357b9496338c6b06adf827550cde2709d61d236 Improve documentation for p13n assets in general (such as the #getInstance method on the Engine): https://github.com/SAP/openui5/commit/0e165315522f7cfcd6f46cee7cda44c95a4d8907, https://github.com/SAP/openui5/commit/f357b9496338c6b06adf827550cde2709d61d236, https://github.com/SAP/openui5/commit/2275b9a63fe6105cb80ccb8c9659d392ef4aabe7

The above mentioned issues have been adressed in these changes as the according type definitions are generated using the jsdoc.

Your understanding is correct, that these keys are freely provided by the application developer through the registration process, but I agree that this needs to be improved in the according detailed explanation. We will improve the central documentation that I linked earlier to make this more clear.

Since the state needs to be translated into according changes for persistence, the only way right now to apply state which will be reflected in the variant management is using the #applyExternalState API. There are however no further changes to be expected in this API, hence we will consider removing the experimental flag in the upcoming release of UI5, but this can unfortunately not be provided as downport to earlier releases. The documentation enhancements mentioned however have been provided as downport for respective releases 1.120 and 1.108 and will be provided with the next patches.

Please get back to me in case of further questions on concerns in regards of this issue.

qualiture commented 6 months ago

Just for my understanding, regarding the state:

Because of the 'dynamic' nature of the state, am I correct in assuming we should implement the parameter signature of the attachStateChange event handler like this:

private registerP13nEngine() {
    // config here

    Engine.getInstance().attachStateChange(this.doTableStateChange.bind(this));
}

private doTableStateChange(event: Event<Record<string, any>, EventProvider>) : void { 
    const state = event.getParameter("state") as { 
        Columns: MetadataObject[],
        Sorter: MetadataObject[],
        Groups: MetadataObject[]
    };

    // etc...
}

It would be nice if we could have a proper typed state object and/or event, but alas...