Tasks #200

Utumno · 2015-03-25T22:01:50Z

Grouping various TODOs that people new to the code can help with. Some are important so more complicated refactorings and new features can take place. Some are just things I can't get round to. Some are things I don't won't to get into. Every little bit helps. Bold items are a priority.

Ongoing - just keep those in mind when editing the code:

Eliminate all except: pass you come across in the code. Have an eye open for bad error handling. See Better error handling #220 - in short NEVER do try..except: pass and NEVER swallow a traceback. See also: https://stackoverflow.com/q/33650696/281545 , https://www.python.org/dev/peps/pep-3151/#appendix-a-survey-of-common-errnos
keep an eye open for "hack", "yak", "fixme" or my signature ##: comments - they mark areas of the code that need work
same goes for local imports (^[ ]+from)
read our style guide and follow it - especially avoid introducing bad names (data, name, key, etc.)
watch https://www.youtube.com/watch?v=OSGv2VnC0go - then put it in (good) use

Easy - no need to know Bash or the games:

Installer_Duplicate needs to be absorbed in File_Duplicate (making it possible to select multiple installers in the process)
~~revisit regexes -> re.U is not needed -> https://docs.python.org/3/library/re.html#re.A~~ moved to Follow up on Python 3 conversion #618
Settings menu option for maximum items open warning
Add a new BAIN skip for DialogueViews files
Deleting an item on a tab should select the next item immediately afterwards - minor QOL tweak
Add a command to remove all .esu plugins to the Mods tab
FNV force-loads Update.bsa - add that to get_bsa_lo

Easy - but you need to know Bash a bit and/or the games:

Add most LSCR subrecords (e.g. NNAM) to the Graphics tag
Double-check all of the default INI edits for Nehrim to see if their values make sense or if Nehrim changed the default from Oblivion
Write some (better) docs for BSA Redirection
On the Installers tab, dragging and dropping an external file onto BAIN should put the archive at the spot where I dragged it, not at the bottom of my 500+ packages list -> Use the x and y parameters of OnDropFiles and do a HitTest with them
Adding on to the 'reselecting after deleting' point above, check if there are any other places in WB where such behavior would be nice
Are there more usages of gui.ListBox where we can set isExtended=True?
Having thousands of saves makes WB excessively slow. Add an option to disable save loading on boot entirely for now (real solution would be making the Saves tab's backing list virtual - see UIList Refactoring: Allow trees instead of just flat lists #529 and 'Data Tab': View of data folder with source installers #50)
Add support for SkyrimCustom.ini and its equivalents in other games

Intermediate:

Hard:

Back/Forward buttons for navigating GUI (similar to xEdit/web browser), support mouse buttons for this too
Add a Relations Checker patcher for Skyrim+ (relation A -> B implies B -> A with the same relationship value)

Done/Moved:

The text was updated successfully, but these errors were encountered:

Motivation was naturally that only mods are ghosted - this was very confusing (one needed to read all the code to verify this is the case) plus inserted ghosting business logic as high up as FileInfos.refresh(). This lead to a rewrite of the rather inadequate TrackedFileInfos API, triggered by refreshFile setting the isGhost attribute in a different way than FileInfo.init. A task from #200.

Under #200. What really triggered this is the erroneous error checks as in `result = ins.close()` - close() does not return a value: http://stackoverflow.com/q/30966953/281545 To understand the solution adopted read: http://stackoverflow.com/a/30984882/281545 Note: - 7zip cmd syntax: http://sevenzip.osdn.jp/chm/cmdline/ - Mopy/bash/basher/settings_links.py: Unswallowing the stacktrace - Moved initializing the 7z.exe path inside initBosh() - Reverted using: - line = unicode(line, Path.sys_fs_enc) + line = unicode(line, 'utf8') (see 698a919 and 32f2447). I passed `u'-scsUTF-8', u'-sccUTF-8'` in the 7z command line instead - neater. - Changed Error regexp: -regErrMatch = re.compile(u'Error: (.*)', re.U).match +regErrMatch = re.compile(ur'Error:', re.U).match would not do the trick. - solid compression produces half the size archives: - # archive srcdir to outFile in 7z format without solid compression + # archive srcdir to outFile in 7z format WITH solid compression but not sure why non solid was used... - on error dump output immediately (later edit): + if regErrMatch(line): + errorLine = line + u''.join(out) + break maCompressing = regCompressMatch(line) - if errorLine or regErrMatch(line): - errorLine.append(line) TODO: - compressCommand must be used _everywhere_ (and needs some commenting and cleanup, maybe not all options needed, see for instance: http://superuser.com/a/444404/35020, http://sourceforge.net/p/sevenzip/discussion/45797/thread/c39971e9/)

Under #200. Another WIP - plus fixes for BCFs, command lines, performance and whatnot - what really triggered this are the erroneous `result = ins.close()` error checking but it proved a hard nut - 7z command line, recursive archives, user ini 7zExtraCompressionArguments etc etc Archives' handling needs to be moved to a module - to understand current solution read: http://stackoverflow.com/q/30966953/281545 http://stackoverflow.com/a/30984882/281545 http://stackoverflow.com/q/31124670/281545 (special thanks to http://stackoverflow.com/users/4279/j-f-sebastian) See 8fc6620 for TODOs - 7z command line needs revisiting to begin with (u'-scsUTF-8' may be redundant for instance) and definitely the progress bars need rethinking - for instance in settings the number of files is counted but it is never used - counting the files takes as much time as unpacking... A design issue is that some stuff is in bosh (exe7z|writeExt|readExt) and some in bolt (subrocess|startupinfo). Needs thought. Signed-off-by: MrD <the.ubik@gmail.com>

Under #200. Only counting files if a progress bar is displayed - counting is _slow_.

Under #200 - now what's more readable ?

@mjpieters

This release features an almost complete refactoring of the codebase, fixing many bugs and opening the way to extend Bash functionality. Bash, being a community work, has over the years become an assortment of hacks, patches and cruft and the program was just about to become non-operable. Example issues - in no particular order (although probably the worst was the horrible performance): - deleting an item in the lists displayed by Bash ran different code depending on whether the user hit delete or right clicked and chose delete - most of those pieces of code were buggy - start up took more than 15 seconds on large Oblivion Load Orders and alt tabbing out and back into Bash would take 6-7 seconds - last one should take no time in the usual case - as seen from the (user visible) debug prints the game detection algorithm run 3-5 times - many, many broken things - including performance in general (most simple operations would hang Bash for some seconds), display glitches and general UI inconsistencies and internal data structures corruption (see #176) Those bugs reflected the state of the code - again in no particular order: - bosh.py (the data model) in 305 was 24635 lines (reduced from 30550 lines in 304.4 - see bosh section below) - basher.py (the UI lists/menus etc) was 18936 lines - the seven tabs that Bash featured were backed up by Tank (2) and List (5) instances - so implementing a feature (say delete) needed edits to at least two classes (usually 7) - the wx library was intermingled with the basher code - the menu items (Links) defined 215 AppendToMenu methods with nearly identical wx code - readability suffered - most attributes/variables were named `data` or `items`, globals hacks crippled the IDE static analysis, copy paste (aka ravioli) code was everywhere. When I started in it I had no idea if refactoring the code was even possible, core Bash is 80k lines of code. It was clear that bosh needed splitting, that candidate number one was the patchers code (well defined and functional) and that basher should follow - but was not at all clear if those were possible, let alone how exactly should the code be structured and how much time would this take. Turns out that it was possible: Bash was indeed coded with modularity in mind (by Wrye ?) but later additions bloated the initial modules beyond recognition and broke the (non enforced) encapsulation - however it wasn't too late. Also turns out that refactoring needed 2 full work years of a single engineer (learning python). The huge performance boost that this release features is the measurable effect of the increase in the code quality. In the items below I try to describe in a quantitative manner what "code quality" means and how it was increased. That's meant to be read from an app that links to the commits - both github and gitk do but additionally github links to the issues and files. ### bosh This release features splitting the patchers' code out of bosh to a dedicated package (whose final structure is still at stake). This reduced bosh to 10516 lines while naturally exposing class dependencies issues, hidden by the "one file program" nature of Bash. The attempt to split bosh had already started in 305, involving: - an effort to eliminate code duplication in C and P patchers using multiple inheritance (chopped off around 1500 lines of pure copy paste code) - see commits in 3c40989 - splitting out what was to become `record_groups.py` - see c3e3543, but don't imitate it - lots has changed since then, including import conventions and commit style - for instance commits on dev that do not launch are strictly forbidden. - splitting out what was to become `parsers.py` - see commits in 08ab6e2 Final (`record_groups.py`, `parsers.py`): 6ae30fc With those out of the way it was finally possible to rip the patchers out of bosh - starting here: 983f259 That was not a mere copy paste rip - warnings were squashed, long lines were wrapped, and most importantly a lot of tricky refactoring was applied to eliminate code duplication - see for ex. commits in: 72fc8a0 for ripping and commits in (note the negative line count): 5d21377, fdab163, 78a85e0 for refactoring See #3 for commit links (finally closed in b6b743b) and #124 for the (still open) package layout issue. Be aware that a lot of things changed since, including import style and the overall package layout - see: e6b2e0e The remaining bosh.py code had some preliminary refactoring applied to it (archives handling, BAIN error handling and refresh etc - see 6ddd4a8) but core bosh refactoring will take place in 307. Major goals are splitting bosh into a number of modules, getting rid of old style classes (DataDict hierarchy, that is the data store singletons) and refactoring settings to efficiently support profiles. ### .data There are 544 occurrences of `.data` in 306 (1081 of `data`) as opposed to 1071 in 305 (2221 `data`) - baring string literals and comments (Pycharm only recently made these scopes available in IDE search, they were much needed). Practically one had to run the debugger to see the type of the object one had at hand, since the variable/attribute name was always 'data': - data is the wrapped dictionary of `DataDict`. Most accesses to this dict were not via the wrappers the DataDict provides. This took many commits to fix - ex 56198be. Before even _thinking_ "performance" read the performance section below (no, none of those modInfos.data.keys() calls was in the middle of a tight loop). - data also was an attribute of the Link hierarchy which had _different meaning based on an isinstance check_: 92e50cd. It could either mean a DataDict subclass (so in Links code there were `self.data.data` accesses) _or_ the list of selected items in the UI, leading to a complete chaos - see the commit above up till the final removal in ffae59e. - UIList subclasses have a `data` attribute that points to the DataDict singletons in bosh. That was dealt with throughout coding 306 as it is closely related to a deeper issue namely the intermingling of UI and model/business logic code. This stems from the 'one file program' nature of Bash so solving this properly means refactoring is at 100%. - to somehow improve readability I introduced idata and pdata attributes for the Installers and People data links - eba53f9 - Those are transitory and meant to also help in eliminating Tank. The links of List subclasses were using self.window.data, whose uses I strived to encapsulate - random example: 0934f1f - all kind of different classes attributes were named `data`. Those required dedicated commits (hey, I couldn't just do a search and replace for "data") - ex. 9839c47. An IDE really helps here. - finally data local variables were renamed - ex. 8e77283 The war with data was going on in parallel with smaller scale wars with `items` (e87dca7 and its parents), `refresh` (4d872ab), etc. ### basher (#163) This is the part of refactoring that I consider almost done - and it was a big pleasure coding it. Since I first tried coding for Bash the dichotomy between balt.Tank and basher.List clearly stood out as a major issue, as did the omnipresence of the wx library and of course the sheer size of basher.py (the UI module). Basher became a package and then the UI API went through a complete rewrite. Basher globals served as a guide in the splitting process, and a metric of class dependencies. Eliminating those globals was an ongoing effort throughout 306 coding - see for instance: 892a19c (screenlist, statusBar), 5852328 (gInstallers), faea771 (modList) till final elimination in bab7c00 Guest star: bosh.links (353be43) Globals served as a link also between Bash's modules (so they evince class and module coupling issues) - eventually encapsulated as static fields of the balt.Link.Frame singleton - see 0690cf3 Link.Frame is an interesting binding of the UI (basher) and the rest of bash that will serve as guide to a later refactoring phase. Especially Link.Frame.modList use in bosh must be eliminated but this is related to the RefreshUI/delete API finalization. The items below detail the UI refactoring. #### wx wx usages went down by an impressive 1148: 305: 2260 usages (balt: 381, basher: 1586) 306: 1112 usages (balt: 418, basher/: 494) Note balt barely going up. This is because, as was stressed in the relevant issue (#190), decoupling the wx library from the code does not simply mean "create wrappers for wx classes/constants and drop them into balt - it means that balt must export an API", so: - balt should not return wx items on which wx methods will be called - see: 9856c69 for example fix. - the code outside of balt should be agnostic of wx ids. Bash code was far form agnostic of wx ids - on the contrary it used them extensively: - balt was exporting it as a function parameter - this mostly resulted in client code calling it with `wx.ID_ANY` (adding wx usages) - see for ex. a555160, 18cf2b1 up till finally dropping the last one of them in 370bef3. - event handlers were using `event.GetId()` - for example fix see 081bfa6. - Link.Execute() has an event parameter which was mostly used with the IdList hack to define lists of menus - this was taken care by the ChoiceLink subclass, introduced in 6fbec32 till finally the last uses of IdList were binned: 000f320 and IdList was removed (967b921) - note that this permitted dropping the `_id` parameter from `Link.__init__`. ItemLink.Execute() `event` parameter is in fact _never used_ in the 306 Link API iteration and will be removed. That's "decoupling wx from Bash code". For commits that combine both fixes see: 46599bb, 9a77bfc The bulk of what remains is to hash up a clever interface for sizers - needs thought as it must accommodate all usages while being powerful enough to cater for new ones. That being said, another thought is incorporating some basher wx dependent stuff back to balt which anyway should be split to a package - at least its bosh depended classes should be clearly separated from its "abstract" wx wrapping API. #### Link API In a gigantic merge (054970e - avoid) the Link subclasses were ripped from basher to per-tab modules - this is not final but it was a necessary step to start splitting basher. As with the rip of patchers this was not a copy paste rip - a new Link class API was introduced. Unlike the patchers rip however (where package structure and the class hierarchy is still at stake) the Link API has few rough edges left in. The guide for designing the Link hierarchy was `AppendToMenu()` calls that featured boilerplate wx code - for wip boilerplate elimination commits see: 6cf40c1 where `EnabledLink` is introduced and 658fcac where my favourite `TransLink` is introduced (AppendToMenu could become quite _complex_ - see also `ChoiceLink`: 6fbec32). The merge chopped off 785 wx uses and AppendToMenu was confined in balt, and reduced to 8 different implementations vs 215 (!) in 305. The Link API significantly evolved since - compare the 306 (semi final) form with the 305 implementation: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/balt.py#L2098-L2114 306: https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/balt.py#L2392-L2446 Previous (old style) Link class explicitly defined _different_ attributes based on an isinstance check, checking if the underlying window was a List or Tank instance. This made the code plain unreadable (given also the names used, see .data section above) while enforcing the Tank/List dichotomy. The isinstance check was finally removed here: 6d260f0. Here is `INI_ListErrors` Link before and after the refactoring: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/basher.py#L11168-L11191 306: https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/basher/ini_links.py#L71-L90 Note AppendToMenu code is encapsulated in EnabledLink, wx wrapper introduced for the clipboard, text and help are now class variables, "data" is now "selected" and the lines are wrapped for more readable, declarative and concise code. For a more complicated example see: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/basher.py#L11029-L11122 306 :https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/basher/mods_links.py#L84-L161 Previous implementation directly manipulated wx IDs (by global ID_*** complex hacks) while the new one declares classes and lets ChoiceLink superclass take care of creating and appending the menu items. This permitted focusing on the Execute code which as seen got improved while refactored to remove duplication. This class still has rough edges (\_self...) which will be corrected in 307 along with rough edges in the API - for one dropping ItemLink.Execute() `event` parameter. Of note that the Link subclasses featured a lot of duplicate code apart from the wx related stuff - see for instance: 1a9a29e for duplicate code in Mod_Export Links. #### UIList After the Links were ripped from basher it became obvious that refactoring could not progress while balt.Tank and basher.List were both still around. That lead, one fine morning, to a new class - UIList: 1462227. Both Tank and List had an attribute (glist and list (!) respectively) pointing to a ListControl instance (the items displayed in the UI). The first UIList merge: abd5f24 (see commits there for how UIList was engineered from common Tank/List code) has List.list removed in favour of gList: fc7bde8, to be finally encapsulated as _gList in c48ce7d (note there is a completely unrelated `gList` patcher attribute). ##### UIList.SortItems() The first real snug though was unifying the sorting API of Tank and List. ListControl does not support sorting, one has to introduce a hacky dictionary mapping indexes to displayed items' ids (not wx ids) - that's what Tank was doing (undocumented !) while List was using no less than PopulateItems - resulting in bad performance most apparent in the ini tab, where sorting would stat the file system (so clicking on a column header Bash would hang couple seconds). I proceeded unifying the List.SortItems() overrides (6f0adc9), then moving the Tank mechanism to the ListControl class where it belongs (52a4a22) and finally moving SortItems to base: 42d213a - note OnColumnClick callback to base and ignore the isinstance(self, Tank) - corrected later. This made also various other fixups around the Sort API possible, namely adding sort indicators in all Bash lists (only mod tabs had them), fixing master lists sorting (and display) and the beginning of TankData param API deprecation: 7a3b872. ##### UIList.RefreshUI() Second snug and a blocker for fixing performance was centralizing (and cleaning up) the RefreshUI family and underlying PopulateItem(s) (UpdateItem(s) in Tank). Some commits: - Moving RefreshUI to List: 39e1e60 - List.PopulateItem(): f390ab7, ecf30e9 - UIList.PopulateItems(): 350075a - Tank and List `__init__()` eliminated: a510b23 Unifying the RefreshUI API made finally possible to remove List itself: d94a09c. More work is done - for instance see some work on refreshing the details displayed in: 3ff6cf2. ##### UIList.DeleteItems() Once List was no more and Tank a relic of itself it was finally possible to tackle the delete API. First relevant merge is: 02a5891 but the API is yet in alpha due to calling UI code from bosh - see: 6452bcb and its parent merge containing 3da60e6 (the ugly DataDict.delete_Refresh()). This API will hit beta once the data refresh methods return deleted, modified and added - see how this precious info is currently discarded: return bool(_added) or bool(_updated) or bool(_deleted) https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/bosh.py#L3887 ### Performance (#123) As seen by profiling, the performance bottleneck was the libloadorder32.dll we bundled with bash. Given the central place load order operations have in a mod manager application, the introduction of a laggy dll had taken Bash's performance down on its knees since 51f99f2. But this was far from being the only problem. Due to the spaghetti nature of the code (throw some event programming in) Bash was very generous with its refreshes: - it would refresh its data three (3) times on boot - RefreshData callback was triggered whenever Bash took focus - _including_ after Bash itself displayed a dialog to the user - on Oblivion, while Bash had refreshed its mod infos it was _always_ requesting also a refresh from libloadorder, to be sure - so, for instance, when just tabbing out and back in to Bash (with absolutely no changes in the Data dir) libloadorder was triggered and went through its refresh cycle - see ad05f44 for the fix - refresh in BAIN, say, install operations was called twice, along with a double call of modList RefreshUI - refreshing the mods panel would result in also refreshing the saves panel - even when the change that triggered the refresh did not affect saves - see commits in: 63d8dec Now, given that most of those refreshes ended up triggering a refresh from the libloadorder32.dll, Bash would take of the order of 10 seconds for all but the most trivial operations. But even if libloadorder was lightning fast the double and triple refreshes were as much a bug as anything - so the dll lag was even a blessing in disguise as it made apparent the underlying chaos. To solve the dll issue one alternative would be to reimplement it in python. But given that the same dll is used by other mod managers and related apps I anyway had to know what's in there. So I ended up forking libloadorder and going through the C++ ~~pain~~ code: https://github.com/Utumno/libloadorder/ I realized that some of the operations performed by the dll are inherently costly and would actually need a complete rewrite using caches - for instance the mod validity checks (see Utumno/libloadorder#3), which by the way involves yet another library and is not clear what is checked (as is not clear what Bash checks and if those checks are basically performed twice - Utumno/libloadorder#6). As the situation was critical in Oblivion mainly (due to stating the filesystem for mod times) I explicitly bypassed the dll because I anyway have all the data needed to determine the load order - 7cd6533 - while I still use the dll for saving it. Liblo 7.6.0 commit (f5de6c8) apart from performance fixups fixes some other issues with liblo 4 and 6 like writing loadorder.txt when not there, return active plugins in order, adding files to libloadorder.txt that are added to Data, etc - see: Utumno/libloadorder@7bb5cfb But rewriting the dll was not enough - on the Python side of things the load order internal API was a giant hack, complete with event handlers and absolutely no encapsulation, let alone proper caches. I introduced a new module `load_order.py` (for first iteration and design decisions see 98ed549) to centralize load_order handling - will be finalized in 307 but already Plugins is again encapsulated in ModInfos which serves as the basic internal load order API for the rest of Bash. Load order API will be final when implementing features such as "undo load order change" is a breeze, but already makes implementing minor load order related features much easier: 14ecafc. Using the caches I introduced I was able to fix performance of getOrdered() - was O(n^3\*logn) instead of O(n\*logn): 5d7ed0b The double and triple refreshes were a deeper issue - the "one file program" thing - so rewriting refreshes (RefreshUI, the DataDicts refresh, RefreshData etc) - was an ongoing effort throughout 306 development, going hand to hand with the UI refactoring (see #163 for RefreshUI and #123 for links to some of the most relevant commits). In short: - the refreshes on bash dialogs were taken care using a decorator to unbind RefreshData(): a71f3f2 - the BAIN issues were addressed finally here: 8107f7b - BAIN however needs to be split in a dedicated package to start properly fixing it. Goals for the next few releases: - cache ini reads - finalize then profile the load order handling API - profile the refresh of data on boot and optimize it - centralize refresh of data and UI so we can eventually - switch to an event based model of runtime refresh (watchdog ?) ### Warnings Pycharm code inspection emits 2228 warnings in 306 as opposed to 4229 in 305. Metrics are from Pycharm 4.5.4 - using this inspection: https://github.com/Utumno/wrye-bash-pycharm/blob/168253bca81313a3cc7dc85ee53747b116e984fb/.idea/inspectionProfiles/default_copy_no_typos.xml on this scope: https://github.com/Utumno/wrye-bash-pycharm/blob/168253bca81313a3cc7dc85ee53747b116e984fb/.idea/scopes/wrye_bash_all_no_cint.xml Warnings come in various sizes and shapes but in general show low code quality. Some I batch fixed (like 'Comparison to None performed with equality operator') but in general I was fixing warnings when visiting nearby code so the remaining ones mostly mark code areas that need attention. On the other end of the spectrum there are warnings that deserve issues of their own (like 'Too broad exception clause' - nicknamed the most diabolical python anti-pattern - see #200). Other interesting cases were: - "Function 'close' doesn't return anything" led to a rewrite of the archives handling code: c29d7b8 - copy paste unused variables were kind of a guide to otherwise heavily undocumented code - but also often pointed to bugs - "method may be static" (264 in 305, went down to 72 in 306) is a classic example of a warning one fixes while editing the code - the remaining ones should be revisited for 307, along with their classes. - unused imports are on their way to extinction - the remaining cases involve `import *` patterns and normalizing the import style, which shall be done when package layout is frozen - and the ugly `__all__` directives one has to update on adding a menu item: 6a2e4a9 - UILists and their links belong together. ### Cruft (#173) Cruft removal got a dedicated issue so returning developers could readily track when and why code they were familiar with got removed. Cruft is not just unused classes, commented out code etc - it is also compatibility with ancient Bash settings (will be removed in 307), non working tabs (like the PM tab also to be removed in 307) and compatibility code with pre 2.7 python (see 6d0a944). One major piece of cruft removed in 306 was BALO: b520591. As you can see removing BALO was needed for tackling refresh - as it was a complex, obsolete piece of code that greatly complicated both refresh and load order handling - and even the Link API (see notoriously complex Mod_BaloGroups(ChoiceLink) subclass here: aa5b89a). ### Formatting & wrapping After some discussion it was clear that PEP8'ing the code would be a pointless effort at the state it was in - however word wrapping is a _functional_ requirement and 79 chars is well thought of, even if you use a wall projector for programming. 305: Total number of non-blank lines: 82463 in 29 files 96.22 lines less than 80 characters (3120 lines exceed) 98.63 lines less than 100 characters (1130 lines exceed) 99.44 lines less than 120 characters (462 lines exceed) 306: Total number of non-blank lines: 85366 in 63 files 97.89 lines less than 80 characters (1803 lines exceed) 99.26 lines less than 100 characters (631 lines exceed) 99.71 lines less than 120 characters (248 lines exceed) Note: - I do not include `cint.py` (and the `chardet` package which should become a submodule) - a big part of the line wrapping was made by manually correcting Pycharm's formatter - so have a close look - the increase of line count is mainly due to 21de440 which adds 5130 lines most (4974) of it being static data in Mopy/bash/game/skyrim.py. Considering there were 34 files added for 34 * 23 = 782 lines of licence text and that wrapping should have produced another 1k lines at least _core Bash code was actually reduced by ~4000 lines_. That is more than welcome and should be imitated - _less code more features_ is a great motto. ### Boot Booting process got a facelift to be finalized in 307. This involves game loading (see 97fc9cf), fixes to the game detection algorithm (e19237c) which used to run 3-5 times on boot, a fix for our custom importer (43e359a) compliments of @mjpieters, and finally a reworking of boot error messages (8dbdd49). ### WINE As reported in #203 "WryeBash v304.2 works beautifully in Wine" - the windows only shell UI stuff broke it though. One of the last things I did for 306 was fixing this - now Bash runs again on WINE: 124f314. Still making it run correctly and in a second phase making Bash run _on bare Linux_ is a code quality goal that got dedicated issues (#240, #241, #243) ------------------------------------------------------------------------ This release is dedicated to Wrye. Special thanks to: - @Sharlikran, @Supierce, @lojack5, @Arthmoor, @alt3rn1ty, @psilocide (aka lefenger), @leandor for testing, support and sharing their knowledge, - @jfpelland, @wduda, @Isenr and @valda for patches, - @mjpieters for saving my sanity at SO a couple times as well as @zed for helping me rewrite the archives handling code. ------------------------------------------------------------------------ # Conflicts: # Mopy/bash/bass.py @@@ -21,9 -21,42 +21,46 @@@ # https://github.com/wrye-bash # # ============================================================================= """This module just stores some data that all modules have to be able to access - without worrying about circular imports.""" + without worrying about circular imports. Currently used to expose layout + and environment issues - do not modify or imitate (ut).""" + import os as _os + import ConfigParser as _cp language = None <<<<<<< HEAD +AppVersion = u"304.4" ======= + AppVersion = u"306" + bashIni = None + + #--Null strings (for default empty byte arrays) + null1 = '\x00' + null2 = null1*2 + null3 = null1*3 + null4 = null1*4 + + def GetBashIni(iniPath=None, reload_=False): ##: needs work + iniPath = iniPath or u'bash.ini' + global bashIni + if reload_ or bashIni is None: + if _os.path.exists(iniPath): + bashIni = _cp.ConfigParser() + bashIni.read(iniPath) + return bashIni + + class Resources: # this belongs to basher but leads to cyclic imports, so... + fonts = None + #--Icon Bundles + bashRed = None + bashBlue = None + bashDocBrowser = None + bashMonkey = None + + # move with its uses to a cross platform 'env.py' module - maybe add bashIni + try: + import _winreg as winreg + except ImportError: + winreg = None >>>>>>> rel-306 ------------------------------------------------------------------------ Build with `2.7.10 (default, May 23 2015, 09:40:32) [MSC v.1500 32 bit (Intel)]` Closes #187.

This release features an almost complete refactoring of the codebase, fixing many bugs and opening the way to extend Bash functionality. Bash, being a community work, has over the years become an assortment of hacks, patches and cruft and the program was just about to become non-operable. Example issues - in no particular order (although probably the worst was the horrible performance): - deleting an item in the lists displayed by Bash ran different code depending on whether the user hit delete or right clicked and chose delete - most of those pieces of code were buggy - start up took more than 15 seconds on large Oblivion Load Orders and alt tabbing out and back into Bash would take 6-7 seconds - last one should take no time in the usual case - as seen from the (user visible) debug prints the game detection algorithm run 3-5 times - many, many broken things - including performance in general (most simple operations would hang Bash for some seconds), display glitches and general UI inconsistencies and internal data structures corruption (see #176) Those bugs reflected the state of the code - again in no particular order: - bosh.py (the data model) in 305 was 24635 lines (reduced from 30550 lines in 304.4 - see bosh section below) - basher.py (the UI lists/menus etc) was 18936 lines - the seven tabs that Bash featured were backed up by Tank (2) and List (5) instances - so implementing a feature (say delete) needed edits to at least two classes (usually 7) - the wx library was intermingled with the basher code - the menu items (Links) defined 215 AppendToMenu methods with nearly identical wx code - readability suffered - most attributes/variables were named `data` or `items`, globals hacks crippled the IDE static analysis, copy paste (aka ravioli) code was everywhere. When I started in it I had no idea if refactoring the code was even possible, core Bash is 80k lines of code. It was clear that bosh needed splitting, that candidate number one was the patchers code (well defined and functional) and that basher should follow - but was not at all clear if those were possible, let alone how exactly should the code be structured and how much time would this take. Turns out that it was possible: Bash was indeed coded with modularity in mind (by Wrye ?) but later additions bloated the initial modules beyond recognition and broke the (non enforced) encapsulation - however it wasn't too late. Also turns out that refactoring needed 2 full work years of a single engineer (learning python). The huge performance boost that this release features is the measurable effect of the increase in the code quality. In the items below I try to describe in a quantitative manner what "code quality" means and how it was increased. That's meant to be read from an app that links to the commits - both github and gitk do but additionally github links to the issues and files. This release features splitting the patchers' code out of bosh to a dedicated package (whose final structure is still at stake). This reduced bosh to 10516 lines while naturally exposing class dependencies issues, hidden by the "one file program" nature of Bash. The attempt to split bosh had already started in 305, involving: - an effort to eliminate code duplication in C and P patchers using multiple inheritance (chopped off around 1500 lines of pure copy paste code) - see commits in 3c40989 - splitting out what was to become `record_groups.py` - see c3e3543, but don't imitate it - lots has changed since then, including import conventions and commit style - for instance commits on dev that do not launch are strictly forbidden. - splitting out what was to become `parsers.py` - see commits in 08ab6e2 Final (`record_groups.py`, `parsers.py`): 6ae30fc With those out of the way it was finally possible to rip the patchers out of bosh - starting here: 983f259 That was not a mere copy paste rip - warnings were squashed, long lines were wrapped, and most importantly a lot of tricky refactoring was applied to eliminate code duplication - see for ex. commits in: 72fc8a0 for ripping and commits in (note the negative line count): 5d21377, fdab163, 78a85e0 for refactoring See #3 for commit links (finally closed in b6b743b) and #124 for the (still open) package layout issue. Be aware that a lot of things changed since, including import style and the overall package layout - see: e6b2e0e The remaining bosh.py code had some preliminary refactoring applied to it (archives handling, BAIN error handling and refresh etc - see 6ddd4a8) but core bosh refactoring will take place in 307. Major goals are splitting bosh into a number of modules, getting rid of old style classes (DataDict hierarchy, that is the data store singletons) and refactoring settings to efficiently support profiles. There are 544 occurrences of `.data` in 306 (1081 of `data`) as opposed to 1071 in 305 (2221 `data`) - baring string literals and comments (Pycharm only recently made these scopes available in IDE search, they were much needed). Practically one had to run the debugger to see the type of the object one had at hand, since the variable/attribute name was always 'data': - data is the wrapped dictionary of `DataDict`. Most accesses to this dict were not via the wrappers the DataDict provides. This took many commits to fix - ex 56198be. Before even _thinking_ "performance" read the performance section below (no, none of those modInfos.data.keys() calls was in the middle of a tight loop). - data also was an attribute of the Link hierarchy which had _different meaning based on an isinstance check_: 92e50cd. It could either mean a DataDict subclass (so in Links code there were `self.data.data` accesses) _or_ the list of selected items in the UI, leading to a complete chaos - see the commit above up till the final removal in ffae59e. - UIList subclasses have a `data` attribute that points to the DataDict singletons in bosh. That was dealt with throughout coding 306 as it is closely related to a deeper issue namely the intermingling of UI and model/business logic code. This stems from the 'one file program' nature of Bash so solving this properly means refactoring is at 100%. - to somehow improve readability I introduced idata and pdata attributes for the Installers and People data links - eba53f9 - Those are transitory and meant to also help in eliminating Tank. The links of List subclasses were using self.window.data, whose uses I strived to encapsulate - random example: 0934f1f - all kind of different classes attributes were named `data`. Those required dedicated commits (hey, I couldn't just do a search and replace for "data") - ex. 9839c47. An IDE really helps here. - finally data local variables were renamed - ex. 8e77283 The war with data was going on in parallel with smaller scale wars with `items` (e87dca7 and its parents), `refresh` (4d872ab), etc. This is the part of refactoring that I consider almost done - and it was a big pleasure coding it. Since I first tried coding for Bash the dichotomy between balt.Tank and basher.List clearly stood out as a major issue, as did the omnipresence of the wx library and of course the sheer size of basher.py (the UI module). Basher became a package and then the UI API went through a complete rewrite. Basher globals served as a guide in the splitting process, and a metric of class dependencies. Eliminating those globals was an ongoing effort throughout 306 coding - see for instance: 892a19c (screenlist, statusBar), 5852328 (gInstallers), faea771 (modList) till final elimination in bab7c00 Guest star: bosh.links (353be43) Globals served as a link also between Bash's modules (so they evince class and module coupling issues) - eventually encapsulated as static fields of the balt.Link.Frame singleton - see 0690cf3 Link.Frame is an interesting binding of the UI (basher) and the rest of bash that will serve as guide to a later refactoring phase. Especially Link.Frame.modList use in bosh must be eliminated but this is related to the RefreshUI/delete API finalization. The items below detail the UI refactoring. wx usages went down by an impressive 1148: 305: 2260 usages (balt: 381, basher: 1586) 306: 1112 usages (balt: 418, basher/: 494) Note balt barely going up. This is because, as was stressed in the relevant issue (#190), decoupling the wx library from the code does not simply mean "create wrappers for wx classes/constants and drop them into balt - it means that balt must export an API", so: - balt should not return wx items on which wx methods will be called - see: 9856c69 for example fix. - the code outside of balt should be agnostic of wx ids. Bash code was far form agnostic of wx ids - on the contrary it used them extensively: - balt was exporting it as a function parameter - this mostly resulted in client code calling it with `wx.ID_ANY` (adding wx usages) - see for ex. a555160, 18cf2b1 up till finally dropping the last one of them in 370bef3. - event handlers were using `event.GetId()` - for example fix see 081bfa6. - Link.Execute() has an event parameter which was mostly used with the IdList hack to define lists of menus - this was taken care by the ChoiceLink subclass, introduced in 6fbec32 till finally the last uses of IdList were binned: 000f320 and IdList was removed (967b921) - note that this permitted dropping the `_id` parameter from `Link.__init__`. ItemLink.Execute() `event` parameter is in fact _never used_ in the 306 Link API iteration and will be removed. That's "decoupling wx from Bash code". For commits that combine both fixes see: 46599bb, 9a77bfc The bulk of what remains is to hash up a clever interface for sizers - needs thought as it must accommodate all usages while being powerful enough to cater for new ones. That being said, another thought is incorporating some basher wx dependent stuff back to balt which anyway should be split to a package - at least its bosh depended classes should be clearly separated from its "abstract" wx wrapping API. In a gigantic merge (054970e - avoid) the Link subclasses were ripped from basher to per-tab modules - this is not final but it was a necessary step to start splitting basher. As with the rip of patchers this was not a copy paste rip - a new Link class API was introduced. Unlike the patchers rip however (where package structure and the class hierarchy is still at stake) the Link API has few rough edges left in. The guide for designing the Link hierarchy was `AppendToMenu()` calls that featured boilerplate wx code - for wip boilerplate elimination commits see: 6cf40c1 where `EnabledLink` is introduced and 658fcac where my favourite `TransLink` is introduced (AppendToMenu could become quite _complex_ - see also `ChoiceLink`: 6fbec32). The merge chopped off 785 wx uses and AppendToMenu was confined in balt, and reduced to 8 different implementations vs 215 (!) in 305. The Link API significantly evolved since - compare the 306 (semi final) form with the 305 implementation: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/balt.py#L2098-L2114 306: https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/balt.py#L2392-L2446 Previous (old style) Link class explicitly defined _different_ attributes based on an isinstance check, checking if the underlying window was a List or Tank instance. This made the code plain unreadable (given also the names used, see .data section above) while enforcing the Tank/List dichotomy. The isinstance check was finally removed here: 6d260f0. Here is `INI_ListErrors` Link before and after the refactoring: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/basher.py#L11168-L11191 306: https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/basher/ini_links.py#L71-L90 Note AppendToMenu code is encapsulated in EnabledLink, wx wrapper introduced for the clipboard, text and help are now class variables, "data" is now "selected" and the lines are wrapped for more readable, declarative and concise code. For a more complicated example see: 305: https://github.com/wrye-bash/wrye-bash/blob/d8f05202e6485a3bef0d92bb55a731b8040eb94e/Mopy/bash/basher.py#L11029-L11122 306 :https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/basher/mods_links.py#L84-L161 Previous implementation directly manipulated wx IDs (by global ID_*** complex hacks) while the new one declares classes and lets ChoiceLink superclass take care of creating and appending the menu items. This permitted focusing on the Execute code which as seen got improved while refactored to remove duplication. This class still has rough edges (\_self...) which will be corrected in 307 along with rough edges in the API - for one dropping ItemLink.Execute() `event` parameter. Of note that the Link subclasses featured a lot of duplicate code apart from the wx related stuff - see for instance: 1a9a29e for duplicate code in Mod_Export Links. After the Links were ripped from basher it became obvious that refactoring could not progress while balt.Tank and basher.List were both still around. That lead, one fine morning, to a new class - UIList: 1462227. Both Tank and List had an attribute (glist and list (!) respectively) pointing to a ListControl instance (the items displayed in the UI). The first UIList merge: abd5f24 (see commits there for how UIList was engineered from common Tank/List code) has List.list removed in favour of gList: fc7bde8, to be finally encapsulated as _gList in c48ce7d (note there is a completely unrelated `gList` patcher attribute). The first real snug though was unifying the sorting API of Tank and List. ListControl does not support sorting, one has to introduce a hacky dictionary mapping indexes to displayed items' ids (not wx ids) - that's what Tank was doing (undocumented !) while List was using no less than PopulateItems - resulting in bad performance most apparent in the ini tab, where sorting would stat the file system (so clicking on a column header Bash would hang couple seconds). I proceeded unifying the List.SortItems() overrides (6f0adc9), then moving the Tank mechanism to the ListControl class where it belongs (52a4a22) and finally moving SortItems to base: 42d213a - note OnColumnClick callback to base and ignore the isinstance(self, Tank) - corrected later. This made also various other fixups around the Sort API possible, namely adding sort indicators in all Bash lists (only mod tabs had them), fixing master lists sorting (and display) and the beginning of TankData param API deprecation: 7a3b872. Second snug and a blocker for fixing performance was centralizing (and cleaning up) the RefreshUI family and underlying PopulateItem(s) (UpdateItem(s) in Tank). Some commits: - Moving RefreshUI to List: 39e1e60 - List.PopulateItem(): f390ab7, ecf30e9 - UIList.PopulateItems(): 350075a - Tank and List `__init__()` eliminated: a510b23 Unifying the RefreshUI API made finally possible to remove List itself: d94a09c. More work is done - for instance see some work on refreshing the details displayed in: 3ff6cf2. Once List was no more and Tank a relic of itself it was finally possible to tackle the delete API. First relevant merge is: 02a5891 but the API is yet in alpha due to calling UI code from bosh - see: 6452bcb and its parent merge containing 3da60e6 (the ugly DataDict.delete_Refresh()). This API will hit beta once the data refresh methods return deleted, modified and added - see how this precious info is currently discarded: return bool(_added) or bool(_updated) or bool(_deleted) https://github.com/wrye-bash/wrye-bash/blob/b40b1a10ff414dd41dd412c8484ca253e42ca92c/Mopy/bash/bosh.py#L3887 As seen by profiling, the performance bottleneck was the libloadorder32.dll we bundled with bash. Given the central place load order operations have in a mod manager application, the introduction of a laggy dll had taken Bash's performance down on its knees since 51f99f2. But this was far from being the only problem. Due to the spaghetti nature of the code (throw some event programming in) Bash was very generous with its refreshes: - it would refresh its data three (3) times on boot - RefreshData callback was triggered whenever Bash took focus - _including_ after Bash itself displayed a dialog to the user - on Oblivion, while Bash had refreshed its mod infos it was _always_ requesting also a refresh from libloadorder, to be sure - so, for instance, when just tabbing out and back in to Bash (with absolutely no changes in the Data dir) libloadorder was triggered and went through its refresh cycle - see ad05f44 for the fix - refresh in BAIN, say, install operations was called twice, along with a double call of modList RefreshUI - refreshing the mods panel would result in also refreshing the saves panel - even when the change that triggered the refresh did not affect saves - see commits in: 63d8dec Now, given that most of those refreshes ended up triggering a refresh from the libloadorder32.dll, Bash would take of the order of 10 seconds for all but the most trivial operations. But even if libloadorder was lightning fast the double and triple refreshes were as much a bug as anything - so the dll lag was even a blessing in disguise as it made apparent the underlying chaos. To solve the dll issue one alternative would be to reimplement it in python. But given that the same dll is used by other mod managers and related apps I anyway had to know what's in there. So I ended up forking libloadorder and going through the C++ ~~pain~~ code: https://github.com/Utumno/libloadorder/ I realized that some of the operations performed by the dll are inherently costly and would actually need a complete rewrite using caches - for instance the mod validity checks (see Utumno/libloadorder#3), which by the way involves yet another library and is not clear what is checked (as is not clear what Bash checks and if those checks are basically performed twice - Utumno/libloadorder#6). As the situation was critical in Oblivion mainly (due to stating the filesystem for mod times) I explicitly bypassed the dll because I anyway have all the data needed to determine the load order - 7cd6533 - while I still use the dll for saving it. Liblo 7.6.0 commit (f5de6c8) apart from performance fixups fixes some other issues with liblo 4 and 6 like writing loadorder.txt when not there, return active plugins in order, adding files to libloadorder.txt that are added to Data, etc - see: Utumno/libloadorder@7bb5cfb But rewriting the dll was not enough - on the Python side of things the load order internal API was a giant hack, complete with event handlers and absolutely no encapsulation, let alone proper caches. I introduced a new module `load_order.py` (for first iteration and design decisions see 98ed549) to centralize load_order handling - will be finalized in 307 but already Plugins is again encapsulated in ModInfos which serves as the basic internal load order API for the rest of Bash. Load order API will be final when implementing features such as "undo load order change" is a breeze, but already makes implementing minor load order related features much easier: 14ecafc. Using the caches I introduced I was able to fix performance of getOrdered() - was O(n^3\*logn) instead of O(n\*logn): 5d7ed0b The double and triple refreshes were a deeper issue - the "one file program" thing - so rewriting refreshes (RefreshUI, the DataDicts refresh, RefreshData etc) - was an ongoing effort throughout 306 development, going hand to hand with the UI refactoring (see #163 for RefreshUI and #123 for links to some of the most relevant commits). In short: - the refreshes on bash dialogs were taken care using a decorator to unbind RefreshData(): a71f3f2 - the BAIN issues were addressed finally here: 8107f7b - BAIN however needs to be split in a dedicated package to start properly fixing it. Goals for the next few releases: - cache ini reads - finalize then profile the load order handling API - profile the refresh of data on boot and optimize it - centralize refresh of data and UI so we can eventually - switch to an event based model of runtime refresh (watchdog ?) Pycharm code inspection emits 2228 warnings in 306 as opposed to 4229 in 305. Metrics are from Pycharm 4.5.4 - using this inspection: https://github.com/Utumno/wrye-bash-pycharm/blob/168253bca81313a3cc7dc85ee53747b116e984fb/.idea/inspectionProfiles/default_copy_no_typos.xml on this scope: https://github.com/Utumno/wrye-bash-pycharm/blob/168253bca81313a3cc7dc85ee53747b116e984fb/.idea/scopes/wrye_bash_all_no_cint.xml Warnings come in various sizes and shapes but in general show low code quality. Some I batch fixed (like 'Comparison to None performed with equality operator') but in general I was fixing warnings when visiting nearby code so the remaining ones mostly mark code areas that need attention. On the other end of the spectrum there are warnings that deserve issues of their own (like 'Too broad exception clause' - nicknamed the most diabolical python anti-pattern - see #200). Other interesting cases were: - "Function 'close' doesn't return anything" led to a rewrite of the archives handling code: c29d7b8 - copy paste unused variables were kind of a guide to otherwise heavily undocumented code - but also often pointed to bugs - "method may be static" (264 in 305, went down to 72 in 306) is a classic example of a warning one fixes while editing the code - the remaining ones should be revisited for 307, along with their classes. - unused imports are on their way to extinction - the remaining cases involve `import *` patterns and normalizing the import style, which shall be done when package layout is frozen - and the ugly `__all__` directives one has to update on adding a menu item: 6a2e4a9 - UILists and their links belong together. Cruft removal got a dedicated issue so returning developers could readily track when and why code they were familiar with got removed. Cruft is not just unused classes, commented out code etc - it is also compatibility with ancient Bash settings (will be removed in 307), non working tabs (like the PM tab also to be removed in 307) and compatibility code with pre 2.7 python (see 6d0a944). One major piece of cruft removed in 306 was BALO: b520591. As you can see removing BALO was needed for tackling refresh - as it was a complex, obsolete piece of code that greatly complicated both refresh and load order handling - and even the Link API (see notoriously complex Mod_BaloGroups(ChoiceLink) subclass here: aa5b89a). After some discussion it was clear that PEP8'ing the code would be a pointless effort at the state it was in - however word wrapping is a _functional_ requirement and 79 chars is well thought of, even if you use a wall projector for programming. 305: Total number of non-blank lines: 82463 in 29 files 96.22 lines less than 80 characters (3120 lines exceed) 98.63 lines less than 100 characters (1130 lines exceed) 99.44 lines less than 120 characters (462 lines exceed) 306: Total number of non-blank lines: 85366 in 63 files 97.89 lines less than 80 characters (1803 lines exceed) 99.26 lines less than 100 characters (631 lines exceed) 99.71 lines less than 120 characters (248 lines exceed) Note: - I do not include `cint.py` (and the `chardet` package which should become a submodule) - a big part of the line wrapping was made by manually correcting Pycharm's formatter - so have a close look - the increase of line count is mainly due to 21de440 which adds 5130 lines most (4974) of it being static data in Mopy/bash/game/skyrim.py. Considering there were 34 files added for 34 * 23 = 782 lines of licence text and that wrapping should have produced another 1k lines at least _core Bash code was actually reduced by ~4000 lines_. That is more than welcome and should be imitated - _less code more features_ is a great motto. Booting process got a facelift to be finalized in 307. This involves game loading (see 97fc9cf), fixes to the game detection algorithm (e19237c) which used to run 3-5 times on boot, a fix for our custom importer (43e359a) compliments of mjpieters, and finally a reworking of boot error messages (8dbdd49). As reported in #203 "WryeBash v304.2 works beautifully in Wine" - the windows only shell UI stuff broke it though. One of the last things I did for 306 was fixing this - now Bash runs again on WINE: 124f314. Still making it run correctly and in a second phase making Bash run _on bare Linux_ is a code quality goal that got dedicated issues (#240, #241, #243) ------------------------------------------------------------------------ This release is dedicated to Wrye. Special thanks to: - Sharlikran, Supierce, lojack5, Arthmoor, alt3rn1ty, psilocide (aka lefenger), leandor for testing, support and sharing their knowledge, - jfpelland, wduda, Isenr and valda for patches, - mjpieters for saving my sanity at SO a couple times as well as zed for helping me rewrite the archives handling code. ------------------------------------------------------------------------ Build with `2.7.10 (default, May 23 2015, 09:40:32) [MSC v.1500 32 bit (Intel)]` Closes #187.

Takes 15 warnings for the ride - the yellow kind. With this commit the only globals hacks that remain are the game/patcher dynamic loading ones plus the tabs creation one - they are more than enough. See also: ccd9839 def _uint(const) is really nitpick - could split and squash with wrapping - did not do finally. Note: - using %r in `__repr__` - better safe... - Mopy/bash/liblo.py: errors had too much in it - no way to get this kind of locals/globals hacks right really - I am using del BSAHandle, LibbsaError, LootDb, LootError to avoid "redeclared without usage" warns. Fragile. Not final - fragile. All this should turn to normal modules, initialized explicitly. Init should be some few lines. In the TODO list. Under #200.

We must factor out common code there - factoring out the hack (that was in the root of #96) for starters Under #200.

- common code duplication in flip esm links (done later) - pass bashtags in ? (calculate once) - pass oblivion collection in ? Is it costly ? - drop rescan mergeable on boot - done later Would have reluctantly added: if fileInfo.header and fileInfo.isEsm(): self.table.getColumn('mergeInfo')[name] = (fileInfo.size, False) in FileInfos - on closer inspection however since flip esm rewrites the header both on memory and on disk just adding the isEsm() check suffices. However we now need to rescan mergeable when flipping to an esp. # This is the 2nd commit message: Rescan\ mergeability on esmp flip: TODOs: - change crc on flip ! (in writeHeader) - cache mergeability info by crc ! Under #200 for refactoring mergeability info by crc.

Archives belong in _bolt_. Closures and nonlocal: http://stackoverflow.com/a/19160431/281545 http://stackoverflow.com/q/3190706/281545 https://docs.python.org/3/reference/simple_stmts.html#the-nonlocal-statement reList did not belong to the installer - it's the standard 7z listing format, moved to bolt - renamed analogous and unrelated vars Any cleanup in Installer class variables very welcome - tmp stuff stands out. Kept `with tempArch...` in clients as I need the unicode safe path (whatever that is). Note that the @balt.conversation does not make a difference - code returns and by the time I close the log RefreshData is long since rebound to BashFrame, and triggered. Maybe just leave it that way ? Under #200 - archives refactoring.

Finally got round to merging the work by @iaz3 (thanks !) into dev. Chops off 200 lines of duplicate code and opens the way to further expand/test/fix the import/export menus. More work is needed to refactor common code in remaining classes, also maybe expand the functionality - and add docs and tests. 4 overrides of _Mod_Import_Link.Execute remain as well as Mod_Fids_Replace and Mod_Face_Import that must be made into _Mod_Import_Link subclasses. Will help with bugs such as #181 too. Under #163, #200. Signed-off-by: MrD <the.ubik@gmail.com>

It was high time - the IDE rearranged the imports in bolt, left them that way. Will come handy when we add more compression stuff - we must transfer all bits of archive handling in here - omods.py has a lot in there. I also threw in a fixup from future inclusion of 7z 16: @@ -2064,3 +2064,4 @@ def countFilesInArchive(srcArch, listFilePath=None, recurse=False): # 3534900 325332 75 files, 29 folders - return int(re.search(ur'(\d+)\s+files,\s+\d+\s+folders', line).group(1)) + # in 7z 16 apparently the folders listing is omitted if 0 + return int(re.search(ur'(\d+)\s+files(,\s+\d+\s+folders)?', line).group(1)) This led to some shadowing - fixed. Btw archives is a rather generic name. Under #200. Under #338.

The goal here is to make ItemLink.help consistent with the other variables around it, by adding an underscore to the start of its name. This commit runs PyCharm's rename refactoring as a starting point for manual renaming of everything it missed. Under wrye-bash#200

See commit <COMMIT_SHA> for Part 1 and the explanation for this change. This commit fixes all (hopefully) remaining usages of the old ItemLink.help in Wrye Bash that were missed in the automatic refactoring in Part 1. Under wrye-bash#200

See commit 0e3912a for Part 1 and the explanation for this change. This commit fixes all (hopefully) remaining usages of the old ItemLink.help in Wrye Bash that were missed in the automatic refactoring in Part 1. Under wrye-bash#200

Mostly the same strategy as with the help text - encapsulate in a property that is queried just before the wx object is created. Along the way we also gained the ability to unghost multiple mods at the same time, a better copy to esp/esm ability (can now copy to esl and esu as well) and the ability to copy/open multiple INIs. Note File_Duplicate deserved the three dots either way, will regularly pop up dialogs even for multiple mods (e.g. when a BSA is attached). No idea why Saves_Profiles.local existed. bosh.saveInfos.localSave works just fine and we use it two lines down anyways. Also contains some work on minimizing the duplication between ESM and ESL flip code. Down to 8 overrides after vs. 22 before. Will not make it into beta7 for obvious reasons.

Tried really hard to fix our progress bars for compression too, but 7zip has made it nigh-on impossible to read progress from its command line. I've just about had it with that stupid binary, maybe it's time to look for alternatives (would also avoid the subprocess usages, context switches, etc.) - py3 however.

Note I named them differently for skyrim - IIUC they are only used in Oblivion code. See: Mopy/bash/game/oblivion/records.py: https://en.uesp.net/wiki/Tes4Mod:Mod_File_Format/ALCH https://en.uesp.net/wiki/Oblivion:Spell_Effects - any idea why REHE is the default? Mopy/bash/game/skyrim/records.py: - https://en.uesp.net/wiki/Tes5Mod:Mod_File_Format/COLL - https://en.uesp.net/wiki/Tes5Mod:Mod_File_Format/SCEN effect_formid Mopy/bash/game/oblivion/constants.py: prefixing Under #200 also

Mopy/bash/basher/gui_patchers.py is mustExist needed in @@ -387,3 +387,4 @@ def OnAdd(self): title = _(u'Get ')+self.__class__.listLabel - srcPaths = balt.askOpenMulti(self.gConfigPanel,title,srcDir, u'', wildcard) + srcPaths = FileOpenMultiple.display_dialog(self.gConfigPanel, title, + srcDir, u'', wildcard) if not srcPaths: return probably not That's one of the remaining recent todos that better be fixed - it's both a wx relic plus Path/filenames/files handling, env/windows coupling etc. This is a very modest start - Note style param was unused for _?askSave and askOpenMulti (and askOpen) - Mopy/bash/basher/mod_links.py: factor out common Path code (apart from _askSave) SSS _askOpen mustExist=True and drop param Mods_ImportBashTags (out of 10 occurrences) left it to default (False) but it opened the file afterwards FFF drop mustExist - but is it needed at all with FD_FILE_MUST_EXIST?? -> I think not so dropped completely

Is it really this simple? (!) Seems we did all the work already. Note this add the wx.TAB_TRAVERSAL style another reason for the ! Infernio: Remove _PopulateItems, now pointless View with whitespace diff off for a clearer diff. Under #200 and the eternal #190.

_recycle gone, and that ancient 'detect shift from menu' <3 Is it under # 172 or is there a newer issue?

One use of ListBoxes down, eight more to go. Brings us some nice features (including the ability to hold shift to delete from the context menu's Delete link as well). Utumno: _recycle gone, and that ancient 'detect shift from menu' <3 Under #172, #200

Utumno modified the milestone: Code Quality Jun 11, 2015

Utumno added a commit that referenced this issue Sep 12, 2015

Dropping noop uses of bolt.Progress() in archives:

b588c07

Under #200. Only counting files if a progress bar is displayed - counting is _slow_.

Utumno mentioned this issue Sep 19, 2015

Questions About Bashs Widgets #66

Closed

Utumno added a commit that referenced this issue Sep 20, 2015

Renaming DocBrowser.data -> DocBrowser.docs:

9e912cb

Under #200 - now what's more readable ?

Utumno added a commit that referenced this issue May 7, 2016

WIP common installer install:

84f8601

We must factor out common code there - factoring out the hack (that was in the root of #96) for starters Under #200.

Utumno mentioned this issue May 15, 2016

Refactoring Backburner #300

Closed

Utumno mentioned this issue Jul 7, 2018

FOMOD Installer Support #380

Closed

Utumno added the M-good-first-issue Misc: A good issue for newcomers to start with label Jan 5, 2019

Utumno referenced this issue Jan 21, 2021

2to3: Run fixer 'dict'

e1e95d0

Infernio mentioned this issue Apr 2, 2021

Collaborators #400

Open

Infernio referenced this issue Jan 5, 2023

fixup! Replace the deletion dialog with a custom one

2745d12

_recycle gone, and that ancient 'detect shift from menu' <3 Is it under # 172 or is there a newer issue?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tasks #200

Tasks #200

Utumno commented Mar 25, 2015 •

edited

Loading

Tasks #200

Tasks #200

Comments

Utumno commented Mar 25, 2015 • edited Loading

Ongoing - just keep those in mind when editing the code:

Easy - no need to know Bash or the games:

Easy - but you need to know Bash a bit and/or the games:

Intermediate:

Hard:

Done/Moved:

Utumno commented Mar 25, 2015 •

edited

Loading