[Discussion] fixing UI bugs

[DavidW]

I've just fixed a UI bug (this one) and I thought it might be a good time to consider generally how we might fix UI bugs in the FP. I can think of four options:

(1) Distribute a new version of ui.menu

Advantages: extremely simple, takes advantage of being first in install stack.

Disadvantages: ui.menu is 25,000 lines long. Distributing a modified version is a maintenance nightmare; the code isn't even faintly self-documenting (you'd have to look using grep or something to find the changes).

(2) Do a carefully-chosen REPLACE_TEXTUALLY on ui.menu

Advantages: simple, reasonably self-documenting.

Disadvantages: you're doing a global search-and-replace on 25,000 lines of live code. I think that's intolerably dangerous.

(3) Replace ui.menu functions via a new .lua file (probably m_cd_fix.lua). 

Advantages: safe, fairly clear, fairly self-documenting. 

Disadvantages: it's awkward to override functions that are still in code via later code; it's not a good mesh with a possible BD adoption. Most importantly, it only works for function updates; it won't work for all fixes.

(4) Use a function library that lets you do targeted edits on ui.menu - e.g. SCS's lib_ui, which is self-contained(!) and has documentation(!!)

Advantages: clear, powerful, save, self-documenting.

Disadvantages: steeper learning curve/more abstract, theoretical risk of bugs in the functions (though I think these are fairly safe).

 

It's probably obvious that I prefer (4), and indeed I've fixed the above bug that way, and uploaded lib_ui and its documentation to the beta. But there are certainly counter-cases to be made.

[Awachi]

As long as the mesh point between coders and users still uses basic English, the machine under the hood just needs to work.

[Bubb]

#1 definitely isn't viable. It would conflict with UI overhauls that drop-in replace UI.MENU, (like LeUI or Dragonspear UI++), and maintaining however many UI.MENU variants for each game / across versions would be hell.

I agree that REPLACE_TEXTUALLY'ing everything is a bad idea. It's too fragile, and too easy to match wrong parts of the file.

I vote for using #3 when possible — it has the least potential of conflicting with mods. #4 is a good backup when #3 isn't possible, however, depending on the edits mods may have to adjust around it. The one big gotcha here is that if we start patching UI.MENU, the only way to stay compatible with UI overhauls is to have them install before the FP, whatever caveats that may bring. (EET and its extensive UI.MENU patching may also be an issue here)

It may also be the case that the FP won't support installing fixes on top of overhauls, I don't know where we would stand on that.

[DavidW]

I’m not too concerned with UI overhaul compatibility, just because it’s a bug fix - hopefully overhauls will fix bugs themselves, and if not the worst consequence is that a given bug fix gets overriden by the overhaul.

In some ways #4 edit: I meant #3 - seems more of a problem for overhauls - even if the overhaul changes a function, the m_cd_fix.lua will overwrite that change.

[CamDawg]

7 hours ago, Bubb said:

#1 definitely isn't viable. It would conflict with UI overhauls that drop-in replace UI.MENU, (like LeUI or Dragonspear UI++), and maintaining however many UI.MENU variants for each game / across versions would be hell.

Keep in mind that EEFP is first in stack, so mods that drop in a new ui.menu will simply overwrite anything from EEFP and (outside of the m_cd_fix.lua method) it wouldn't matter how EEFP makes changes.

I guess a better starting point might be to get an idea of how many fixes ui.menu really needs from someone like you or @lefreut . I'd advocate a very different approach based on whether we're taking 2 or 3 fixes vs. dozens and/or massive, systematic fixes.

[kjeron]

I vote to leave UI-overhauls to incorporate any such fixes on there end, as they would most likely have to do so anyway.

#3 Would make it a lot harder for non-overhaul mods to patch UI.menu, as the relevant code may no longer be there.

@DavidW I take it the only difference between #1, #2 & #4 would be how the patch is coded, with (ideally) the same resulting UI.Menu?

 

To add to the list of UI.menu fixes: IWDEE needs scrolling in the sequencer menu, to display more than 12 memorized spell for a given level.

[DavidW]

30 minutes ago, kjeron said:

@DavidW I take it the only difference between #1, #2 & #4 would be how the patch is coded, with (ideally) the same resulting UI.Menu?

Yes.

[lefreut]

4 hours ago, CamDawg said:

I guess a better starting point might be to get an idea of how many fixes ui.menu really needs from someone like you or @lefreut . I'd advocate a very different approach based on whether we're taking 2 or 3 fixes vs. dozens and/or massive, systematic fixes.

I don't think you will need a lot of UI fixes (the confirmation prompt fix for IWDEE is the only one that I can think of right now).

For small changes, targeted edits is probably the best. You may need another approach for bigger changes but most of these changes will fall into the tweaks category anyway so not for EEFixpack.

Edited April 19, 2022 by lefreut

[CamDawg]

52 minutes ago, lefreut said:

I don't think you will need a lot of UI fixes (the confirmation prompt fix for IWDEE is the only one that I can think of right now).

This puts strongly in favor of option one: ship a fixed ui.menu.

[DavidW]

To repeat my point from a Discord discussion: we wouldn’t make a change to an itm file in NI and then ship the file, even if there’s only that one change being made.

(there are points of analogy and disanalogy- I’m not meaning it to be decisive.)

[lefreut]

1 hour ago, CamDawg said:

This puts strongly in favor of option one: ship a fixed ui.menu.

A fixed UI.menu will almost always be broken if one day a 2.7 is released. For such a small fix, a patch version is more robust.