BUG-FIX: Multicolview uncheck behavior

[marijncv]

Signed-off-by: Marijn Valk marijncv@hotmail.com

What changes are proposed in this pull request?

This bugfix makes the MultiColumnView react to the checked/unchecked columns by updating the visible columns. Partially resolves #4819

How is this patch tested?

Go to the experiment UI, switch to the multi column view and select/unselect some columns and see them become visible/invisible

Release Notes

Is this a user-facing change?

• No. You can skip the rest of this section.
• Yes. Give a description of this change to be included in the release notes for MLflow users.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

What component(s), interfaces, languages, and integrations does this PR affect?

Components

• area/artifacts: Artifact stores and artifact logging
• area/build: Build and test infrastructure for MLflow
• area/docs: MLflow documentation pages
• area/examples: Example code
• area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
• area/models: MLmodel format, model serialization/deserialization, flavors
• area/projects: MLproject format, project running backends
• area/scoring: MLflow Model server, model deployment tools, Spark UDFs
• area/server-infra: MLflow Tracking server backend
• area/tracking: Tracking Service, tracking client APIs, autologging

Interface

• area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
• area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
• area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
• area/windows: Windows support

Language

• language/r: R APIs and clients
• language/java: Java APIs and clients
• language/new: Proposals for new client languages

Integrations

• integrations/azure: Azure and Azure ML integrations
• integrations/sagemaker: SageMaker integrations
• integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

• rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
• rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
• rn/feature - A new user-facing feature worth mentioning in the release notes
• rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
• rn/documentation - A user-facing documentation change worth mentioning in the release notes

[dbczumar]

@sunishsheth2009 Can you take a look here? I know we implemented a fix for this on Databricks, but it would be nice to include this test coverage.

@marijncv Thanks a bunch for this fix, particularly the comprehensive test coverage!

[sunishsheth2009]

What is required for? Maybe I am missing something but I can't find any reference for maintainColumnOrder.

[marijncv]

I saw it mentioned here in the documentation of the Ag Grid. But I think I interpreted it incorrectly. I was trying to deal with the issue in which a re-checked column would appear at the end of the column list instead of in the place where it was before. See an example below where version and user have been unchecked and rechecked and ended up at the end of the list.

In any case, i'll remove the maintainColumnOrder property here.

[sunishsheth2009]

I think this is a good change to do, so that when we check and uncheck columns the order is maintained. Feel free to add it back or we can add it another PR. :)
Didn't know this API existed. Thank you for pointing to the API.

[marijncv]

@sunishsheth2009 thanks for your reply regarding the maintainColumnOrder. Unfortunately, setting that property to true on the grid does not solve the ordering issue. The only way i've been able to solve it is to call setColumnDefs twice. First with an empty array and right after with the correct columnDefs. This workaround is also mentioned here and here.

I don't particularly like the workaround, but at least it makes the unchecking/checking of columns have the desired effect on the MultiColView. What do you think?

[sunishsheth2009]

@sunishsheth2009 thanks for your reply regarding the maintainColumnOrder. Unfortunately, setting that property to true on the grid does not solve the ordering issue. The only way i've been able to solve it is to call setColumnDefs twice. First with an empty array and right after with the correct columnDefs. This workaround is also mentioned here and here.

I don't particularly like the workaround, but at least it makes the unchecking/checking of columns have the desired effect on the MultiColView. What do you think?

@marijncv can we push the change that we used at databricks to fix this issue to your branch? Your tests do makes sense and are great, but is it ok if we change the JS implementation logic a bit?

[marijncv]

@sunishsheth2009 sure, go ahead!

[sunishsheth2009]

@sunishsheth2009 sure, go ahead!

I have pushed the changes, please let me know your thoughts on the changes and has it fixed the issue that you were trying to solve. Thank youi

[marijncv]

Works like a charm! Thanks @sunishsheth2009!

[dbczumar]

Thanks @marijncv for the awesome submission + test coverage & thanks @sunishsheth2009 for the tweaks! If there are no objections (let me know @sunishsheth2009), let's merge this!

[sunishsheth2009]

Let's merge this!!! Thank you @dbczumar and @marijncv :)