De-wx'ing

[Utumno]

All wx code must eventually end up in ~~balt~~ gui. This does not simply mean that one must create wrappers for wx classes/constants and drop them into gui - it means that gui must export an API - so gui should not return wx items on which wx methods will be called - must wrap those methods into class methods it exports - in other words we must not only eliminate wx imports but also wx calls on wx objects returned by gui functions (such as Bind). This (apart its obvious, I hope, value vis a vis code quality) will also make it easier to move to later versions of wxPython / offer an entirely different backend. There is already a great deal of it done (edited out of this issue). But there is also an awful lot left to be done. So:

• sizers - that's the biggest one. According to what I wrote above, defining say wxEXPAND = wx.EXPAND in gui is not the way to go. Rather the usage patterns of sizers in Bash must be explored from someone who groks the wx sizers API and an API be crafted in gui which defines what one wants (as opposed to how to achieve it).
  • [x] wx.LEFT etc, 0 does make any cense ?
  • [x] I would like to go through the uses of sizers a bit and see what is left out - the wx.TOP etc stuff for instance - I have a plan for adding them in the LayoutOptions
  • [x] rename default_border etc to item_border etc - this is confusing (and much too verbose)
• an Images API is also badly needed - I hacked up a couple classes but that's exactly that - a hack - see 36e7f8973183dbe16699d8789626663580538fb8. -> new issue #366
• [x] rest of functions in balt to classes (like checkBox, v/hsizer etc)
• A single package with wx imports. Single and as small as possible -> gui. Files that currently import wx (and they shouldn't) are:
  • [ ] basher/__init__.py: 7 usages left (mostly wx.Notebook but also wx.EVT_CONTEXT_MENU, wx.NOT_FOUND, wx.Platform, wx.EVT_NOTEBOOK_PAGE_CHANGED)
  • [x] constants.py: nasty, our settings defaults depended on wx constants
  • [x] dialogs.py: sizers, wx.TE_RICH2, wx.ColourPickerCtrl, wx.Icon
  • [x] frames.py:
    • [x] this needs attention. wx.lib.iewin --> this must go! Also a lot of classes in need of wrapping
    • [x] wx.ArtProvider_GetBitmap (icons API !)
    • [x] wx.FlexGridSizer
    • [x] One random usage of wx.NOT_FOUND
  • [x] gui_fomod.py: same problem as belt below
  • [x] gui_patchers.py: sizers, wx.Window
  • [x] patcher_dialog.py: sizers, event binding
  • [x] settings_links.py: sizers And outside basher:
  • [x] belt.py: the wizard dialog
  • [x] id parameters from balt functions/constructors must be dropped
  • [x] balt functions such as button must become classes and be inherited by OkButton, CancelButton etc

Some newer TODOs:

• [x] where did the tracebacks go? -> fixed in 6f121cb432f13c39d67c07dcabf512d17dcb3ada
• [x] the issue with warnings on booting still allowing the user to interact with main frame -> fixed on wx4 yey! -> https://trac.wxwidgets.org/ticket/17816
• [x] the progress dialog - ugh. Our whole CancelError stuff is not working anymore (and I never liked that exception control flow anyway). ~~More importantly, there is the strange issues of losing focus and I think it's the progress window again.~~ -> wx bug, fixed by switching to GenericProgressDialog -> see #728
• [x] related to the above - make sure we don't refresh twice - we must follow a bit the events (with good old prints) and make sure they are processed once - I am thinking also of cases we display dialogs and such, is RefreshData triggered? More @balt.conversation ? We could maybe add an _is_shown attribute and use the evt_activate argument to only refresh when the _is_shown becomes True from False -> seems working ok but keep an eye open!
• [x] not sure about properties yet - sometimes it's ok but sometimes methods are preferred - more explicit - WIP but stabilizing - current code is fine
• [ ] events - it seems wx did not doubly register a handler - see notes on 4766e1051b1e2f534a24b474dfd2cf2c20cf8aec. We do and we skip by default which means we have to go through the previous uses and carefully evaluate if skip is needed. Seems it's not the default because it is not meant to be used often. So there are system handlers that might need to be skipped - could not really understand their docs: https://wxpython.org/Phoenix/docs/html/events_overview.html#how-events-are-processed - I guess system handlers run after user ones
  • [ ] revisit event handling during boot - especially size events need be handled after setting sizes before maximizing apparently etc - see 4d70274ad9a8791723d2e683b7b5467a8809b540 (Attempt to fix installers tab positions not being set)
• [ ] balt will no longer serve a purpose once we're done - all balt code should either go into basher (highly specific Wrye Bash code) or into gui (pure abstractions over wx). Useful rule of thumb: if you think the class / method could be easily copied into another project without dragging core Bash code along while still being useful, it's gui
• [ ] revisit locale handling - see> wxWidgets/Phoenix#1702 from https://github.com/wrye-bash/wrye-bash/issues/15#issuecomment-690801208

Under #163

[Utumno]

To the backburner for now

[Utumno]

@barcharcraz - have a look at this one and at the eternal #163 and #174 to get a feeling of how far we are now from the code 4 years ago where basically the wx library was everywhere. Still work to do but far from the situation in 054970e7b4d82e351c9a696b8e3c9750516d96b8

[Utumno]

@Infernio I have kept a close eye (rather nose cause I am on mobile usually) on your tour-de-force with taking over from @nycz the de-wx stuff. I can't really comment (due to my limited means + the size of the endeavour). Just couple observations:

• __slots__ are not really needed here (except if I miss something) methinks. I am sure you know all about them so I won't repeat all the blah blah on slots being a memory opt ( https://stackoverflow.com/a/28059785/281545 ), needed when creating millions of instances - in Bash, slots are used in the records code (probably as an optimization or due to the dynamic nature, ahem, of records classes) - this predates me. They are also used by me in the bsa files and save header hierarchies - I used them as kind of internal docs as those are binary files that have a limited and precise number of attributes, so kinda felt natural. They are used in other places (Installer, currently broken, for unpickling - btw MreEffect is also breaking the records slots). So there may be some design/style reasons to use them but those classes are plain old python classes that do need the limitations of slots probably.
• which brings us to "if in doubt leave it out" - great talk on APIs design: https://www.youtube.com/watch?v=heh4OeB9A-c

I like very much the idea of wrapping the events but see talk above - briefly one must use an API to evaluate it.

---

I still believe that we need to first address the webview/iewin as a first step (see . If this locks us to wx 2.9 then high time for a nightly with 2.9 ;)

[Infernio]

Yeah, the slots are probably not needed. I threw them in cause I had no idea just how many instances of the GUI classes we create, but it really isn't that many.

Regarding 2.9, it probably is high time for a nightly with 2.9. 2.8 is untenable, since my BitmapButton replacement would have to be completely rewritten to support 2.8 (and in doing so would lose the ability to show both text and an image).

[Infernio]

Slots are gone now, also cleaned up everything that was commented on.

[Utumno]

Thanks! another thing that bothers me btw is my UIList being a wx Panel - should also be a wrapper of the wx control __gList but one step at a time

On Wed, Aug 21, 2019, 23:39 Infernio [email protected] wrote:

Yeah, the slots are probably not needed. I threw them in cause I had no idea just how many instances of the GUI classes we create, but it really isn't that many.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/wrye-bash/wrye-bash/issues/190?email_source=notifications&email_token=AAKNIVYWCNP6IHNERLLW4ELQFWRXNA5CNFSM4A3KLFJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD43BWPA#issuecomment-523639612, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKNIV7XIDOUQIW3R6OXGNDQFWRXNANCNFSM4A3KLFJQ .

[Utumno]

@Infernio - link to SO question on wx locale - https://stackoverflow.com/q/54178028/281545

On Thu, Aug 22, 2019, 19:27 Mr&Mrs D [email protected] wrote:

Thanks! another thing that bothers me btw is my UIList being a wx Panel - should also be a wrapper of the wx control __gList but one step at a time

On Wed, Aug 21, 2019, 23:39 Infernio [email protected] wrote:

Yeah, the slots are probably not needed. I threw them in cause I had no idea just how many instances of the GUI classes we create, but it really isn't that many.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/wrye-bash/wrye-bash/issues/190?email_source=notifications&email_token=AAKNIVYWCNP6IHNERLLW4ELQFWRXNA5CNFSM4A3KLFJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD43BWPA#issuecomment-523639612, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKNIV7XIDOUQIW3R6OXGNDQFWRXNANCNFSM4A3KLFJQ .

[Utumno]

@lojack5 @Infernio I pushed the wx part of lojack-py3 branch on dev - if those changes are wx3 compatible we should (squash) merge those on dev (except if they are contained already in wx-begone branch) - under #460 too (edit: dunno how I unassigned nycz btw ?)

[Infernio]

dunno how I unassigned nycz btw ?

Huh. Normally github does that for people who no longer have a connection to the project. He's still part of the org though, so that's not it: https://github.com/orgs/wrye-bash/people

Edit: I think github is having hiccups, now it swallowed both of us :P

Ah, it's because 1. nycz never commented on this issue and 2. he's an org member but doesn't have read/write permissions to this repo:

You can assign up to 10 people to each issue or pull request, including yourself, anyone who has commented on the issue or pull request, anyone with write permissions to the repository, and organization members with read permissions to the repository**.

From here: https://help.github.com/en/github/managing-your-work-on-github/assigning-issues-and-pull-requests-to-other-github-users

[Infernio]

Left some comments on that branch. Pretty sure only 36471b38f760f16c1c98b75fd20a692e1e6936ce, 5dab3fa60f97d3edc6a0270a5a28949d1c53b448 and 4257102245adfab0169022d389f48d2d640f732f are backwards-compatible, and we need to test if 6bcd48fbcf864c2fc9eb52556fffacf3c9f3f069 is still needed with inf-wx-begone.

[Utumno]

Thanks! I leave it to you to do the squashing - will turn the eye on wx-begone with your permission will stuff there my couple commits from my records branch - wx stuff (including the wx-begone one) are our current blocker for py3, plus it's already on nightly for a while, I just want to have a more careful look before merging

[Infernio]

will turn the eye on wx-begone with your permission will stuff there my couple commits from my records branch

Yup, feel free We should think about merging (at least part of) wx-begone soon as a 190-de-wx-part-1 merge, since no-ff'ing a 30+ commit branch would look pretty silly. I put in quite a bit of effort to make sure none of the commits break dev as long as they are merged in sequence, so you could merge e.g. the first 10 commits without breaking dev, but dropping any of those commits would probably break dev.

[Utumno]

Let's have one last squash on nightly to check my commits also (if you agree with fill -> expand) and then merge time - haven't really reviewed, but I believe the wrappers now are as powerful as wx, with the probable addition of with_border to LayoutOptions (to allow for different borders on top/btm/right/left) - so we're good to go

[Infernio]

if you agree with fill -> expand

Yeah sure, looks fine Will squash and build one more nightly with those changes

[Utumno]

Finally got round to working with the new APIs in wx-begone - must say it's a pleasurable experience :)

We should focus on merging, as much depends on it but I would like some feedback on the commits I (force, hope you don't mind!) pushed on inf-wx-begone - especially the changes on the _AComponent api.

The ones in utumno-wx-begone-broken are broken (haven't got round to tracing the 'tree' ListBoxes) but show how these new apis could be used to encapsulate some of the tougher nuts, like Dialogs and ListBoxes

[Infernio]

Left some preliminary comments on that branch, and here are some thoughts on utumno-wx-begone-broken:

probably this class should be moved to gui/

Yup, my (still pretty far-off...) vision is to have every direct abstraction over wx ugliness in gui, leaving us basher as a kind-of still pretty WIP 'controller'. Not sure about balt yet, it might just disappear with its contents split into gui/basher as appropriate? Would mean we get to drop a silly name...

maybe the time of the ListDataEditor has come

Yes please :) That class is... not the prettiest (see also #484)

CheckListBox is probably needed instead of stuffing all its methods to ListBox

Definitely, that many methods make the API almost impossible to grasp.

And regarding the whole API for that - I heavily used properties for _AComponent, mostly because that's how the code I inherited from nycz worked. But I actually quite like how it looks now - so we should probably make a decision on whether (future?) components should try to use properties first and methods second as well.

Edit: one more thing, I really don't like the kwargs fishing in ListBox, so I'd be in favor of dropping that junk and simply adding them all as keyword arguments (better IDE support, better linting...).

[Utumno]

Will move new classes to gui/misc_components (?) in next iteration - I agree with the vision:)

Yes please :) That class is... not the prettiest (see also #484)

Why you hate so much my little closure that can't go out by it-self_ ? :P Revisiting ListDataEditor is not easy to bin and actually simplifies ListEditor - the real issue is encapsulating the index handling better spend our energy there - an OrderedDict that maps the data model items (sometimes Paths) to their string values displayed by the ListBoxes could be a solution to that

whether (future?) components should try to use properties first and methods second as well.

I reserve properties for when a property is needed - a function is most explicit (there is some processing going on) - existing properties in _AComponent seem ok for now I would say

Edit: one more thing,

Done!

[Infernio]

Why you hate so much my little closure that can't go out by it-self_ ? :P

Hmm, I think my brain went 'nope' the first time I saw the self_ = self assignment - plus pycharm was screaming warnings at me when I first saw that class - but I'll defer to you here :)

[Utumno]

I have done a big rebase of this branch but last minute I saw your fixup - should I cherry pick and force push?

I split your fixups and tried to make commits self contained for merge and fixed a load of bugs style or otherwise. I think you will find lots of interesting stuff - random:

• wizards size not remembered present also on dev
• double binding of refresh?
• some work on the API seems pretty much there

in all looks nice :)

[Infernio]

My fixups are just two lines, so feel free to push and cherry-pick them afterwards.

[Utumno]

Pushed and pushed again - will be rebasing again soon have a look :)

[Utumno]

New iteration - stable enough for nightly. Among others fixes double binding of RefreshData which IIUC was not a problem in the previous implementation but is a problem now. Could write volumes but the rebase exhausted me :P - well, the basic reason I started this was because I needed to use the new APIs to see if they are up to the task. In general, they were a pleasure to use and are quite powerful. Some TODOs: -> moved to the OP

[Metallicow]

@Utumno According to robin... Refresh does not immediately cause a paint event to be sent. It just marks the window as "needing to be painted" and then the system will get around to sending the actual paint event when the time is right.

...Basically this is right, so if you expect Refresh to automatically update.. then that isnt always the case. Note that doing a Refresh then Update usually does. Also send size event or PostSizeEvent normally refeshes the painted window with out the user having to grab a gripper(tho, this is tricky when app is fullscreen or minimized) so note that too. Don't want you doing a bit of work only to have you reply "why did it not work like I expected"

[Utumno]

@Metallicow thanks I was talking about RefreshData not the wx Refresh

@Infernio wait before new nightly will have to do a couple fixups (and a new rebase)

[Utumno]

Done :)

[Infernio]

Something in the recent app button refactoring seems to have broken the game button's behavior when a script extender is installed.

Specifically, if no extender is installed and you click the button, the regular launcher should run. If an extender is installed, then there will be a green checkbox in the status bar. If that is not ticked, then the regular launcher should run, same as above.

If it is ticked, then the extender's binary (e.g. skse64_launcher.exe for SKSE64) should directly be run, enabling the extender and skipping the launcher. This last part doesn't seem to work anymore, instead the regular launcher is ran even with the green checkbox ticked.

On 24. Feb 2020, at 19:37, Utumno [email protected] wrote:

Done :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[Utumno]

I see in the source:

        exePath = (self.exePath if not exeObse.exists() and
                   self.exePath.tail == u'Oblivion.exe' else exeObse)

Did this ever work for other games than oblivion? :P Although this means that it always launch **SE for other games

I had a go at replacing splitters and pushed a fixup for those issues too goodnightzzzz

[Metallicow]

I don't know because I don't use total conversions or mods like that, but that would be an interesting point to investigate to rewrite that line of code.... It does appear to raise questions the way the logic is written...

[Utumno]

belt is broken in this WIP and scratching my head as to why - at some point we should return a PageInstaller from RunLine btw whose return value is never used

Rebased and pushed it should be ready to test - moved my belt attempts at: ut-belt-xxx branch

[Utumno]

Well figured out but prepare for a horrible hack that I moreover can't make to work - testing with

Kill The Orchestra V3.1-52462-3-1.7z.zip

on skyrim

Turns out we need to monkey-patch the native widget's GetNext/Prev - but although works in the debugger does not work in practice showing a Finish button that works as Next however

Have a look on my branchy ut-belt-xxx