Skip to content

Adds multi-select support#1360

Open
jtreminio wants to merge 11 commits into
mcmonkeyprojects:masterfrom
jtreminio:multi-select
Open

Adds multi-select support#1360
jtreminio wants to merge 11 commits into
mcmonkeyprojects:masterfrom
jtreminio:multi-select

Conversation

@jtreminio
Copy link
Copy Markdown
Contributor

@jtreminio jtreminio commented Apr 30, 2026

  • works across batch and history containers, synced to each other.
  • can be easily hooked into (see Adding Image Comparison #1345)
  • when enabled, clicking a card description selects the card
CleanShot.2026-04-29.at.23.31.21.mp4

@jtreminio jtreminio marked this pull request as ready for review April 30, 2026 04:51
@jtreminio jtreminio marked this pull request as draft April 30, 2026 22:21
@jtreminio jtreminio marked this pull request as ready for review April 30, 2026 23:45
let imageHistoryBrowser = new GenPageBrowserClass('image_history', listOutputHistoryFolderAndFiles, 'imagehistorybrowser', 'Thumbnails', describeOutputFile, selectOutputInHistory,
`<label for="image_history_sort_by">Sort:</label> <select id="image_history_sort_by"><option>Name</option><option>Date</option></select> <input type="checkbox" id="image_history_sort_reverse"> <label for="image_history_sort_reverse">Reverse</label> &emsp; <input type="checkbox" id="image_history_allow_anims" checked autocomplete="off"> <label for="image_history_allow_anims">Allow Animation</label>`);
imageHistoryBrowser.enableBrowserMultiSelect = true;
imageHistoryBrowser.keepBrowserMultiSelectKeyAfterPrune = function(key, namesInCurrentList) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

match existing patterns for function declarations


function clickImageInBatch(div) {
let imgElem = div.getElementsByTagName('img')[0];
let multiSelectKey = getImageFullSrc(div.dataset.src);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there shouldn't be any multiselect handling outside of the browser class like this

Comment thread src/wwwroot/js/genpage/gentab/models.js Outdated
}
let isStarred = this.isStarred(model.data.name);
let starButton = { label: isStarred ? 'Unstar' : 'Star', onclick: () => { this.toggleStar(model.data.name); } };
let starButton = { label: isStarred ? 'Unstar' : 'Star', onclick: () => { this.toggleStar(model.data.name); }, can_multi: true };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you objecting to the , can_multi: true flag itself (how I've defined it, for example), or objecting to Star/Unstar action being available for multi-select action?

Copy link
Copy Markdown
Member

@mcmonkey4eva mcmonkey4eva May 2, 2026

Choose a reason for hiding this comment

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

you editing the way star is handled for multi, which is already covered properly for images, and presets/models should handle it the same as images

Copy link
Copy Markdown
Member

@mcmonkey4eva mcmonkey4eva May 2, 2026

Choose a reason for hiding this comment

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

Probably the best move is limit the PR to browser itself & images, preset/models can be separate

aka the universal rule of PR contributing: minimize the scope of edits per individual PR

toggleStar(fullsrc, src);
}
},
can_multi: true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no

if (div) {
removeImageBlockFromBatch(div);
}
if (imageHistoryBrowser.enableBrowserMultiSelect) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is questionably placed aaand also seems to be a mixup between the 'enable' and 'active' bools?

return true;
}
let currentImageBatchDiv = getRequiredElementById('current_image_batch');
for (let candidate of currentImageBatchDiv.querySelectorAll('.image-block:not(.image-block-placeholder)')) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is an odd query

{ label: 'Direct Apply', onclick: () => applyOnePreset(preset.data) },
{ label: preset.data.is_starred ? 'Unstar' : 'Star', onclick: () => togglePresetStar(preset) },
{ label: 'Direct Apply', onclick: () => applyOnePreset(preset.data), can_multi: true },
{ label: preset.data.is_starred ? 'Unstar' : 'Star', onclick: () => togglePresetStar(preset), can_multi: true },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no

{ label: 'Edit Preset', onclick: () => editPreset(preset.data) },
{ label: 'Duplicate Preset', onclick: () => duplicatePreset(preset.data) },
{ label: 'Export Preset', onclick: () => exportOnePresetButton(preset.data) },
{ label: 'Export Preset', onclick: () => exportOnePresetButton(preset.data), can_multi: true },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this can't function natively and needs separate impl

this.runAfterUpdate = [];
this.refreshHandler = (callback) => callback();
this.checkIsSmall();
this.enableBrowserMultiSelect = false;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should probably be allowMultiSelect

this.checkIsSmall();
this.enableBrowserMultiSelect = false;
this.multiSelectActive = false;
this.multiSelectedKeys = new Set();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is liable to lead to state tracking inconsistencies, as jank as it feels putting class or .dataset's on the block divs is probably the stablest way to track

textBlock.tabIndex = 0;
textBlock.innerHTML = desc.description;
div.appendChild(textBlock);
div.addEventListener('click', (e) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

don't need a separate click handler, the select handler should cover it

Comment on lines +68 to +76
let getMeta = (metadata) => metadata ? (JSON.parse(metadata) || {}) : {};
let metaParsed = getMeta(metadata);
let isStarred = (e) => {
let currentMeta = e && e.dataset ? getMeta(e.dataset.metadata) : {};
if (Object.keys(currentMeta).length == 0) {
currentMeta = metaParsed;
}
return currentMeta.is_starred;
};
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There was a bug in your original implementation:

  1. metadata would initially not exist on an item, causing exception on let metaParsed = JSON.parse(metadata);
  2. metadata would be stale on reads below. Clicking "Enable Starred" would add the needed metadata and also star the items, but then clicking "Disable(d) Starred" would still see stale metadata and not unstar items
  3. Fixed speling

@jtreminio jtreminio requested a review from mcmonkey4eva May 2, 2026 16:06
let textBlock = createDiv(null, 'model-descblock');
textBlock.tabIndex = 0;
textBlock.innerHTML = desc.description;
textBlock.addEventListener('click', (e) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's with all these new click listeners?

Copy link
Copy Markdown
Contributor Author

@jtreminio jtreminio May 3, 2026

Choose a reason for hiding this comment

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

This one is necessary. I couldn't need get multi-select click to select a card if I clicked the description otherwise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

correct

}
let items = [];
for (let child of this.contentDiv.children) {
if (child.dataset && child.dataset.name && child.classList.contains('browser-multiselect-item-selected')) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

querySelectorAll should do this quick n simple

}

/**
* Labels for bulk actions shared by every selected item, respecting `can_multi` / `multi_only`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feels like AI slop. Misinterpreted keywords and then proudly documented respect for the misunderstood keyword.

if (files.length == 0) {
return [];
}
let eligiblePerFile = [];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually wtf is this whole function? Why was this built like this at all??? wat.

@mcmonkey4eva
Copy link
Copy Markdown
Member

Issues from testing:

  • Multi-select overlaps with regular select. The image I have selected looks like it is selected for multi-select but isn't actually. This is weird. Probably kill any singleselect classes.
  • After testing, I see some value to having a set like you did before: state is currently killed on any refresh, so need to persist past that. I'd minimize the set handling to be exclusively just a tool to reapply the selected class after a refresh (grab a set of what elems have the class before, apply back after, call it a day)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants