Refactor facet handling in dlgBarAndPieChart by ARMSTRONGOPONDO · Pull Request #10074 · IDEMSInternational/R-Instat

ARMSTRONGOPONDO · 2025-11-12T11:48:53Z

this pull request partially fixes issue #10035

@lilyclements @berylwaswa @anastasia-mbithe Replaced clsFacetRowOp and clsFacetColOp with clsVarsFunction to streamline row and column variable management. Updated dlgBarAndPieChart_Load to initialize clsVarsFunction. Refactored UpdateParameters and facet_grid logic to use clsVarsFunction for rows and cols parameters.

Ive changed the logic but i keep getting the previous syntax on the R output maybe you can recommend a different dataset i can use to test it with @lilyclements

Replaced `clsFacetRowOp` and `clsFacetColOp` with `clsVarsFunction` to streamline row and column variable management. Updated `dlgBarAndPieChart_Load` to initialize `clsVarsFunction`. Refactored `UpdateParameters` and `facet_grid` logic to use `clsVarsFunction` for `rows` and `cols` parameters.

gemini-code-assist · 2025-11-12T11:49:08Z

Summary of Changes

Hello @ARMSTRONGOPONDO, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request aims to modernize and simplify the internal handling of facet variables within the dlgBarAndPieChart dialog. By consolidating the management of row and column facet operators into a single clsVarsFunction, the change reduces redundancy and improves the clarity of how R's facet_grid parameters are constructed. This refactoring enhances maintainability and prepares the codebase for more robust facet-related features.

Highlights

Facet Handling Refactor: Replaced the separate clsFacetRowOp and clsFacetColOp operator classes with a single clsVarsFunction to centralize and streamline the management of row and column variables for faceting.
Initialization and Configuration: The new clsVarsFunction is now initialized in dlgBarAndPieChart_Load and InitialiseDialog, and configured to use the 'ggplot2' package's 'vars' command.
Parameter Update Logic: The UpdateParameters method has been refactored to remove obsolete parameter handling for the old row/column operators and now correctly utilizes clsVarsFunction for setting 'cols' and 'rows' parameters in facet grid operations.
Facet Grid Integration: The facet_grid logic has been updated to directly use clsVarsFunction when adding 'rows' and 'cols' parameters, simplifying how facet variables are passed to the R command.
Typo Correction: Corrected a recurring typo from AddRemoveCountlabel to AddRemoveCountLabel across multiple method calls.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request refactors the facet handling in dlgBarAndPieChart to use clsVarsFunction, aligning with ggplot2's vars() syntax for facet_grid. This is a good improvement. The changes are mostly correct, but I've identified a couple of issues with the implementation for facet_grid parameters and a regression in the group_by functionality. I've provided detailed comments and code suggestions to address these. The other changes, including typo fixes, look good.

gemini-code-assist · 2025-11-12T11:50:55Z

instat/dlgBarAndPieChart.vb

        If bRowAll OrElse bColAll Then
-            clsFacetFunction.AddParameter("margins", "TRUE")
+            clsFacetFunction.AddParameter("margin", "TRUE", iPosition:=1)
        Else
-            clsFacetFunction.RemoveParameterByName("margins")
+            clsFacetFunction.RemoveParameterByName("margin")
        End If
+
        If bRow OrElse bRowAll Then
-            clsFacetVariablesOperator.AddParameter("left", clsROperatorParameter:=clsFacetRowOp, iPosition:=0)
-        ElseIf (bCol OrElse bColAll) AndAlso bWrap = False Then
-            clsFacetVariablesOperator.AddParameter("left", ".", iPosition:=0)
+            clsFacetFunction.AddParameter("rows", clsRFunctionParameter:=clsVarsFunction, iPosition:=0)
        Else
-            clsFacetVariablesOperator.RemoveParameterByName("left")
+            clsFacetFunction.RemoveParameterByName("rows")
        End If
+
        If bCol OrElse bColAll Then
-            clsFacetVariablesOperator.AddParameter("right", clsROperatorParameter:=clsFacetColOp, iPosition:=1)
-        ElseIf (bRow OrElse bRowAll) AndAlso bWrap = False Then
-            clsFacetVariablesOperator.AddParameter("right", ".", iPosition:=1)
+            clsFacetFunction.AddParameter("cols", clsRFunctionParameter:=clsVarsFunction, iPosition:=0)
        Else
-            clsFacetVariablesOperator.RemoveParameterByName("right")
+            clsFacetFunction.RemoveParameterByName("cols")
        End If


There are a couple of issues with the facet_grid parameters:

The parameter for margins in ggplot2::facet_grid is margins, not margin.

You are using iPosition:=0 for both rows and cols parameters. This can lead to an unpredictable order of arguments in the generated R code. It's better to use distinct positions to ensure correctness.

The iPosition for margins (margin in your code) is 1, which could conflict with cols if cols were correctly positioned.

I suggest the following changes to fix these issues:

If bRowAll OrElse bColAll Then clsFacetFunction.AddParameter("margins", "TRUE", iPosition:=2) Else clsFacetFunction.RemoveParameterByName("margins") End If If bRow OrElse bRowAll Then clsFacetFunction.AddParameter("rows", clsRFunctionParameter:=clsVarsFunction, iPosition:=0) Else clsFacetFunction.RemoveParameterByName("rows") End If If bCol OrElse bColAll Then clsFacetFunction.AddParameter("cols", clsRFunctionParameter:=clsVarsFunction, iPosition:=1) Else clsFacetFunction.RemoveParameterByName("cols") End If

gemini-code-assist · 2025-11-12T11:50:55Z

instat/dlgBarAndPieChart.vb

                Select Case ucrInputStation.GetText()
                    Case strFacetWrap
                        GetParameterValue(clsFacetVariablesOperator)
-                    Case strFacetCol, strFacetColAll
-                        GetParameterValue(clsFacetColOp)
-                    Case strFacetRow, strFacetRowAll
-                        GetParameterValue(clsFacetRowOp)
+
                End Select


The logic to populate clsGroupByFunction for facet_grid (cases strFacetCol and strFacetRow) seems to be missing after the refactoring. This will break the group_by functionality when using facet_grid.

You should add back the cases for strFacetCol and strFacetRow and populate clsGroupByFunction from clsVarsFunction. Since GetParameterValue only accepts an ROperator, you'll need to either inline the logic or refactor GetParameterValue to accept an RCodeStructure.

Here is a suggestion to inline the logic:

Select Case ucrInputStation.GetText() Case strFacetWrap GetParameterValue(clsFacetVariablesOperator) Case strFacetCol, strFacetColAll, strFacetRow, strFacetRowAll Dim i As Integer = 0 For Each param As RParameter In clsVarsFunction.clsParameters If param.strArgumentValue <> "" AndAlso param.strArgumentValue <> "." Then clsGroupByFunction.AddParameter(i, param.strArgumentValue, bIncludeArgumentName:=False, iPosition:=i) i += 1 End If Next End Select

lilyclements · 2025-11-12T13:19:17Z

instat/dlgBarAndPieChart.vb

-        clsFacetRowOp.SetOperation("+")
-        clsFacetRowOp.bBrackets = False
-        clsFacetColOp.SetOperation("+")
-        clsFacetColOp.bBrackets = False


I can see here when I compare to @anastasia-mbithe's changes in dlgBoxPlot (#10071) that she adds in

clsFacetFunction.AddParameter("facets", clsROperatorParameter:=clsFacetVariablesOperator, iPosition:=0)

Is there a reason this is missing? Can you go through comparing yours to hers to see what the differences are? I have put both side by side to see on my screen (see screenshot above).

Run through them side by side to see what changes she has that you do not, and vice versa.

Also, this is a really good learning point -- if there is any code on here that you do not understand, then you should ask (even if it's most of it!). This is the time to really understand the VB code so that you can become a real expert in it!

@lilyclements , that line is still there on his side(616), for my dialogs I just moved it up so that its readable and someone doesn't miss it when looking at the function

My apologies, thanks @anastasia-mbithe

anastasia-mbithe · 2025-11-13T07:53:22Z

@ARMSTRONGOPONDO I just checked out your branch and its working okay. I assume you tested without looking at the facet options, and the first is facet wrap, which hasn't changed its function and still uses the old version with ~, but if you test the Facet Row, or Column or Row +O, Col+O, which uses facet_grid it should work with the updated arguments.

Please test again from your end and see if its updated

lilyclements · 2025-11-13T17:12:00Z

@ARMSTRONGOPONDO have you been able to test this?

Refactored `dlgBarAndPieChart.vb` and `dlgGeneralForGraphics.vb` to improve parameter management and streamline logic for `facet_wrap` and `facet_grid`. Removed redundant operators (`clsFacetRowOp`, `clsFacetColOp`) and replaced them with `clsVarsFunction` for better consistency and maintainability. Simplified the initialization and updating of facet-related parameters, ensuring cleaner and more modular code. Enhanced readability and reduced redundancy by consolidating logic for handling `rows`, `cols`, and `margin` parameters.

ARMSTRONGOPONDO · 2025-11-17T09:47:11Z

@lilyclements I have done the changes and tested it @anastasia-mbithe can you also test the two dialogs the dlgbarandpiechart and dlggeneralforgraph work as expected

lilyclements

This is great @ARMSTRONGOPONDO! Well done. I've left a tiny change for you to make.
I'm not sure if it were something already in the VB code when you got to it, and not a big deal as R does partial-matching of strings, but, the margins parameter is margins, not margin

lilyclements · 2025-11-17T11:26:58Z

instat/dlgBarAndPieChart.vb

+                clsFacetFunction.SetRCommand("facet_grid")
+
+                If bRowAll OrElse bColAll Then
+                    clsFacetFunction.AddParameter("margin", "TRUE")


This should be margins not margin

lilyclements · 2025-11-17T11:27:15Z

instat/dlgGeneralForGraphics.vb

+        clsFacetFunction.RemoveParameterByName("facets")
+        clsFacetFunction.RemoveParameterByName("rows")
+        clsFacetFunction.RemoveParameterByName("cols")
+        clsFacetFunction.RemoveParameterByName("margin")


Again here - correct as margins not margin

lilyclements · 2025-11-17T11:27:26Z

instat/dlgGeneralForGraphics.vb

-            clsFacetVariablesOperator.RemoveParameterByName("left")
-        End If
+            If bRowAll OrElse bColAll Then
+                clsFacetFunction.AddParameter("margin", "TRUE")


Again here - correct as margins not margin

lilyclements · 2025-11-17T11:27:32Z

instat/dlgBarAndPieChart.vb

+        clsFacetFunction.RemoveParameterByName("facets")
+        clsFacetFunction.RemoveParameterByName("rows")
+        clsFacetFunction.RemoveParameterByName("cols")
+        clsFacetFunction.RemoveParameterByName("margin")


ARMSTRONGOPONDO · 2025-11-17T11:32:52Z

This is great @ARMSTRONGOPONDO! Well done. I've left a tiny change for you to make. I'm not sure if it were something already in the VB code when you got to it, and not a big deal as R does partial-matching of strings, but, the margins parameter is margins, not margin

Noted let me implemented the change

lilyclements · 2026-01-06T15:46:47Z

@ARMSTRONGOPONDO what is the state of this? Did you implement the change? Have you tested this?

ARMSTRONGOPONDO · 2026-01-06T16:16:30Z

@lilyclements I implemented the changes and tested it's ready for review

lilyclements · 2026-01-06T16:22:24Z

@anastasia-mbithe can you review this? I believe you've been involved in the facet side of things

lilyclements · 2026-03-04T13:38:21Z

@anastasia-mbithe have you been able to look at this?

gemini-code-assist bot reviewed Nov 12, 2025

View reviewed changes

lilyclements reviewed Nov 12, 2025

View reviewed changes

lilyclements reviewed Nov 17, 2025

View reviewed changes

updated margin parameter to margins

19951b5

Merge branch 'master' into facets

3804f8a

Conversation

ARMSTRONGOPONDO commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Nov 12, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

lilyclements Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

anastasia-mbithe Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

lilyclements Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

anastasia-mbithe commented Nov 13, 2025

Uh oh!

lilyclements commented Nov 13, 2025

Uh oh!

ARMSTRONGOPONDO commented Nov 17, 2025

Uh oh!

lilyclements left a comment

Choose a reason for hiding this comment

Uh oh!

lilyclements Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

lilyclements Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

lilyclements Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

lilyclements Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

ARMSTRONGOPONDO commented Nov 17, 2025

Uh oh!

lilyclements commented Jan 6, 2026

Uh oh!

ARMSTRONGOPONDO commented Jan 6, 2026

Uh oh!

lilyclements commented Jan 6, 2026

Uh oh!

lilyclements commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ARMSTRONGOPONDO commented Nov 12, 2025 •

edited

Loading

lilyclements commented Mar 4, 2026 •

edited

Loading