Skip to content

Add collapsible sections to tool parameters #306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
t0mdavid-m wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add collapsible sections to tool parameters #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/Workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ def configure(self) -> None:
)
with t[0]:
# Parameters for FeatureFinderMetabo TOPP tool.
# Example of conditional_sections: Show algorithm:mtd section only when
# algorithm:common:chrom_fwhm has a specific value. The controlling parameter
# must NOT be inside the section it controls.
# conditional_sections={
# "algorithm:mtd": {
# "param": "algorithm:common:chrom_fwhm",
# "value": [5.0, 10.0] # Show section when param matches any value
# }
# }
self.ui.input_TOPP(
"FeatureFinderMetabo",
custom_defaults={"algorithm:common:noise_threshold_int": 1000.0},
Expand Down
114 changes: 108 additions & 6 deletions src/workflow/StreamlitUI.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,7 @@ def input_TOPP(
display_subsections: bool = True,
display_subsection_tabs: bool = False,
custom_defaults: dict = {},
conditional_sections: dict = {},
) -> None:
"""
Generates input widgets for TOPP tool parameters dynamically based on the tool's
Expand All @@ -557,6 +558,12 @@ def input_TOPP(
display_subsections (bool, optional): Whether to split parameters into subsections based on the prefix. Defaults to True.
display_subsection_tabs (bool, optional): Whether to display main subsections in separate tabs (if more than one main section). Defaults to False.
custom_defaults (dict, optional): Dictionary of custom defaults to use. Defaults to an empty dict.
conditional_sections (dict, optional): Dictionary controlling section visibility based on parameter values.
Keys are section names (or prefixes that match subsections). Values are dicts with:
- "param": The controlling parameter name (must NOT be inside the controlled section).
- "value": Value or list of values that make the section visible.
Example: {"algorithm:mtd": {"param": "algorithm:common:enable_mtd", "value": "true"}}
Sections not in this dict are always displayed normally.
"""

if not display_subsections:
Expand Down Expand Up @@ -691,6 +698,84 @@ def show_subsection_header(section: str, display_subsections: bool):
),
)

def get_param_value(param_name: str):
"""Get the current value of a parameter by its short name (e.g., 'algorithm:common:param')."""
for p in params:
short_name = p["key"].decode().split(":1:")[1]
if short_name == param_name:
return p["value"]
Comment on lines +701 to +706
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_param_value function performs a linear search through all parameters each time it's called. This is invoked for every section via should_show_section(). Consider building a parameter lookup dictionary once before the loop to avoid O(n) lookups for each section, especially when there are many sections.

Suggested change
def get_param_value(param_name: str):
"""Get the current value of a parameter by its short name (e.g., 'algorithm:common:param')."""
for p in params:
short_name = p["key"].decode().split(":1:")[1]
if short_name == param_name:
return p["value"]
# Build a lookup table for parameters by their short name to avoid
# repeatedly scanning the full `params` list in get_param_value().
param_lookup: dict[str, dict] = {}
for _p in params:
short_name = _p["key"].decode().split(":1:")[1]
# Preserve previous behavior of returning the first matching entry
# in case of duplicate short names.
if short_name not in param_lookup:
param_lookup[short_name] = _p
def get_param_value(param_name: str):
"""Get the current value of a parameter by its short name (e.g., 'algorithm:common:param')."""
p = param_lookup.get(param_name)
if p is not None:
return p["value"]

Copilot uses AI. Check for mistakes.
return None

def get_controlling_section(section: str) -> dict | None:
"""
Check if a section (or its parent) is controlled by conditional_sections.
Returns the control config dict if found, None otherwise.
"""
if not conditional_sections:
return None
# Check exact match first
if section in conditional_sections:
return conditional_sections[section]
# Check prefix matches (e.g., "algorithm" controls "algorithm:common")
for controlled_section in conditional_sections:
Comment on lines +719 to +720
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prefix matching logic may return the wrong control config when multiple controlled sections share a common prefix. For example, if conditional_sections contains both "algorithm" and "algorithm:common", checking "algorithm:common:param" could match "algorithm" first depending on dictionary iteration order. Consider sorting controlled sections by length in descending order before checking, or use more specific matching logic.

Suggested change
# Check prefix matches (e.g., "algorithm" controls "algorithm:common")
for controlled_section in conditional_sections:
# Check prefix matches (e.g., "algorithm" controls "algorithm:common").
# When multiple keys share a prefix (e.g., "algorithm" and "algorithm:common"),
# prefer the most specific (longest) match by sorting keys by length descending.
for controlled_section in sorted(conditional_sections, key=len, reverse=True):

Copilot uses AI. Check for mistakes.
if section.startswith(controlled_section + ":"):
return conditional_sections[controlled_section]
return None

def should_show_section(section: str) -> tuple[bool, bool]:
"""
Determine if a section should be shown and whether to use an expander.

Returns:
(should_show, use_expander)
- (True, True): Show in expander (controlled section, value matches)
- (True, False): Show normally (not controlled)
- (False, False): Hide entirely (controlled section, value doesn't match)
"""
control = get_controlling_section(section)
if control is None:
return (True, False) # Not controlled, show normally

controlling_param = control.get("param")
expected_values = control.get("value")

# Normalize expected_values to a list
if not isinstance(expected_values, list):
expected_values = [expected_values]

current_value = get_param_value(controlling_param)

# Check if current value matches any expected value
if current_value in expected_values:
return (True, True) # Show in expander
# Handle string comparison for booleans
Comment on lines +747 to +751
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When current_value is None (parameter not found), the function still converts it to string "none" and checks against expected values. This could lead to unintended matches if "none" or "None" is in the expected values list. Consider explicitly handling the None case before performing comparisons.

Suggested change
# Check if current value matches any expected value
if current_value in expected_values:
return (True, True) # Show in expander
# Handle string comparison for booleans
# If the controlling parameter is not found (None), do not match any expected value
if current_value is None:
return (False, False) # Hide entirely when controlling parameter is missing
# Check if current value matches any expected value
if current_value in expected_values:
return (True, True) # Show in expander
# Handle string comparison for booleans and case-insensitive matches

Copilot uses AI. Check for mistakes.
if str(current_value).lower() in [str(v).lower() for v in expected_values]:
return (True, True)

return (False, False) # Hide entirely

def get_section_label(section: str) -> str:
"""Create a formatted label for expander from section name."""
if not section or section == "all":
return "Parameters"
parts = section.split(":")
return ":".join(parts[:-1]) + (":" if len(parts) > 1 else "") + f"**{parts[-1]}**"
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label formatting logic creates markdown with bold formatting for the last part. However, when there's only one part (len(parts) == 1), it returns **part** without any prefix, but when there are multiple parts, it adds a trailing colon before the bold part. This creates inconsistent formatting. Consider simplifying to always return the formatted string consistently regardless of the number of parts.

Suggested change
return ":".join(parts[:-1]) + (":" if len(parts) > 1 else "") + f"**{parts[-1]}**"
if len(parts) == 1:
# Single-part section: just bold the name
return f"**{parts[0]}**"
# Multi-part section: show prefix, then bold the last part
prefix = ":".join(parts[:-1])
return f"{prefix}:**{parts[-1]}**"

Copilot uses AI. Check for mistakes.

# Validate that controlling parameters are not inside their controlled sections
if conditional_sections:
for controlled_section, control in conditional_sections.items():
controlling_param = control.get("param", "")
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the controlling parameter name is empty (when "param" key is missing from the control dict), controlling_param_section will also be empty, and the validation check on lines 771-772 may not detect all invalid configurations. Consider adding validation to ensure both "param" and "value" keys exist in control configs before proceeding.

Suggested change
controlling_param = control.get("param", "")
# Ensure control config contains required keys
if not isinstance(control, dict) or "param" not in control or "value" not in control:
st.error(
f"Configuration error: Conditional section '{controlled_section}' "
"must define both 'param' and 'value' keys."
)
return
controlling_param = control.get("param")
# controlling_param must be a non-empty string
if not isinstance(controlling_param, str) or not controlling_param.strip():
st.error(
f"Configuration error: Controlling parameter for section "
f"'{controlled_section}' must be a non-empty string."
)
return

Copilot uses AI. Check for mistakes.
# Extract section from controlling param (e.g., "algorithm:common:param" -> "algorithm:common")
controlling_param_section = ":".join(controlling_param.split(":")[:-1])
# Check if controlling param is inside the controlled section
if (controlling_param_section == controlled_section or
controlling_param_section.startswith(controlled_section + ":")):
st.error(
f"Configuration error: Controlling parameter '{controlling_param}' "
f"cannot be inside the section it controls ('{controlled_section}')."
)
return

def display_TOPP_params(params: dict, num_cols):
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter params is annotated as dict but is actually a list of parameter dictionaries based on how it's called (e.g., section_params on line 878). The type annotation should be list instead of dict.

Suggested change
def display_TOPP_params(params: dict, num_cols):
def display_TOPP_params(params: list[dict[str, Any]], num_cols):

Copilot uses AI. Check for mistakes.
"""Displays individual TOPP parameters in given number of columns"""
cols = st.columns(num_cols)
Expand Down Expand Up @@ -779,16 +864,33 @@ def display_TOPP_params(params: dict, num_cols):
print('Error parsing "' + p["name"] + '": ' + str(e))


for section, params in param_sections.items():
for section, section_params in param_sections.items():
should_show, use_expander = should_show_section(section)

if not should_show:
continue # Hide section entirely
Comment on lines +867 to +871
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new conditional section visibility logic lacks test coverage. The feature includes multiple code paths (exact match, prefix match, value matching, validation errors) that should be tested to ensure correct behavior, especially the validation logic on lines 765-777 and the section visibility decisions.

Copilot uses AI. Check for mistakes.

if tabs is None:
show_subsection_header(section, display_subsections)
display_TOPP_params(params, num_cols)
if use_expander:
label = get_section_label(section)
help_text = section_descriptions.get(section)
with st.expander(label, expanded=True, help=help_text):
display_TOPP_params(section_params, num_cols)
else:
show_subsection_header(section, display_subsections)
display_TOPP_params(section_params, num_cols)
else:
tab_name = section.split(":")[0]
with tabs[tab_names.index(tab_name)]:
show_subsection_header(section, display_subsections)
display_TOPP_params(params, num_cols)

if use_expander:
label = get_section_label(section)
help_text = section_descriptions.get(section)
with st.expander(label, expanded=True, help=help_text):
display_TOPP_params(section_params, num_cols)
else:
show_subsection_header(section, display_subsections)
display_TOPP_params(section_params, num_cols)

self.parameter_manager.save_parameters()


Expand Down
Loading