Skip to content
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.

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

Add per-user configurable top-level tab hiding #238

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

neggles
Copy link

@neggles neggles commented Sep 2, 2024

hi fren, it's your worst nightmare, I actually did the thing! and it even accounts for dynamic/extension-added tabs!

well okay it only sort of works. by which I mean it does not work.

the magicks for dropdown popovers appear to be broken with select2's multi-select (since nothing else seems to ever actually call makeMultiselectInput()?) and I am not enough of a JS person to work out how to fix that. So the multiselect shows up, but it's styled wrong, and changing the selected options doesn't mark the option as altered so it's not saveable.

But the hiding does work if I manipulate things in the browser debugger to pretend like the server returned a list of tab IDs to hide.

what do, how fix?

src/Core/WebServer.cs Outdated Show resolved Hide resolved
src/Core/WebServer.cs Outdated Show resolved Hide resolved
src/Core/WebServer.cs Outdated Show resolved Hide resolved
@@ -514,6 +515,22 @@ public static async Task<JObject> SetParamEdits(Session session, JObject rawData
return new JObject() { ["success"] = true };
}

public static async Task<JObject> RegisterTabs(Session session, JObject rawData)
Copy link
Member

Choose a reason for hiding this comment

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

the heck are clients registering tabs for? That's... backwards. Server should define them.

Copy link
Author

Choose a reason for hiding this comment

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

Because of this:

<a class="nav-link active translate" data-bs-toggle="tab" href="#Text2Image" id="text2imagetabbutton" aria-selected="true" role="tab">Generate</a>
</li>
@WebServer.T2ITabHeader
<li class="nav-item" role="presentation">
<a class="nav-link translate" data-bs-toggle="tab" href="#utilities_tab" id="utilitiestabbutton" aria-selected="false" tabindex="-1" role="tab">Utilities</a>

unless i am mistaken extensions can add arbitrary extra <li> elements (or any html actually) to the tab list via this, and there's no server side registry of what's added - that variable is just An String. Enforcing that extensions provide some kind of ExtensionTab record rather than An String would be an API break and I ain't about to go parsing arbitrary extension-provided HTML in server side code

if i am mistaken/didn't look hard enough and this html is generated somewhere from an object rather than being arbitrary shit provided by an estension that would be great

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's entirely generated, WebServer#GatherExtensionPageAdditions generates that, and knows exactly what titles/ids are in use.

Copy link
Author

Choose a reason for hiding this comment

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

well that'll teach me to go modifying an unfamiliar codebase at 1:30am.

ok lemme just. go right ahead and make this less insane

@mcmonkey4eva
Copy link
Member

mcmonkey4eva commented Sep 2, 2024

Multi-select inputs work as intended locally:
image

They look a bit weird but that's mostly just, I never bothered making them fit in well because only a couple params use it.

Scorers and LoRA params are the only multiselects I think exist offhand. Definitely no settings that use em atm, so might need to adjust some code related to Settings to make it process those properly. (Or be lazy and just do a comma separated list as a text input as I did for reuse params exclude list)

@mcmonkey4eva
Copy link
Member

please don't force push

@neggles
Copy link
Author

neggles commented Sep 3, 2024

@mcmonkey4eva ok, 2 more force-pushes to undo the first one sry about that 😅

src/Core/Settings.cs Outdated Show resolved Hide resolved
@neggles neggles marked this pull request as ready for review September 6, 2024 09:58
@neggles
Copy link
Author

neggles commented Sep 6, 2024

OK, made that change and un-drafted this since it's not nearly as cursed now.

[edit] ignore that tiny force push i was missing two spaces

}

/// <summary>Register a top-level nav tab from an extension.</summary>
/// <param name="ID">The tab button's <a> element ID.</param>
Copy link
Member

Choose a reason for hiding this comment

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

I think this <a> will yield a C# xml doc error?

@mcmonkey4eva
Copy link
Member

per Discord discussion yesterday: should swap this down to just a CSV input for now.

I posted an issue #258 to track the need to replace select2 with something that doesn't suck

@neggles
Copy link
Author

neggles commented Oct 4, 2024

Still haven't gotten to this but will attempt to in the near future 😅

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