Renaming saves/screens/installers fixups #259

Utumno · 2015-12-01T21:02:36Z

Backburner - see 20b66f1 for todos and issue #300 :

renaming auto/quicksaves.bak is broken - cofiles are not moved along (heck they are not detected to begin with)
screenshots/installers reselect (and details) !
factor out common code in OnLabelEdited

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

Utumno · 2015-12-04T16:49:42Z

@Lojack: the : is really puzzling me - moreover when I try to reproduce in the debugger it disappears - a "heisenbug". It is related to the : being the drive separator and actually moveTo fails here:

wrye-bash/Mopy/bash/bolt.py

Line 967 in 98c729f

os.makedirs(destPath._shead)

- it tries to create a dir in a non existent drive - EDIT: that's where it fails in the debugger - in normal operation there is no traceback - while when I run the debugger I get:

__init__.py 6464 _rename: Renaming F:\Oblivion Mods2\Bash Installers\888888 - Copy - Copy.rar to p:.rar failed
Traceback (most recent call last):
  File "bash\bosh\__init__.py", line 6461, in _rename
    oldPath.moveTo(newPath)
  File "bash\bolt.py", line 967, in moveTo
    os.makedirs(destPath._shead)
  File "C:\_\Python27\lib\os.py", line 157, in makedirs
    mkdir(name, mode)
WindowsError: [Error 3] The system cannot find the path specified: u'p:'

That's from the installers (same story as screens)

But why it is deleting the file is beyond me - actually the file is replaced with a zero bytes filename if I try to rename it to filename:.

EDIT2: Well I think I hit a bug - I asked here (with a reproducer outside Bash): http://stackoverflow.com/q/34097936/281545. Now on to patching Bash

EDIT3: no bug, feature - see answer in the question linked

Under #259 Previous code was no op - still renaming policy must be refactored out! It's all over Bash. Dont do this: `digits = len(str(some_number))` in a loop - see: http://stackoverflow.com/a/2189827/281545 Also we need more generic filename chars checking (belongs to env) but for starters: http://stackoverflow.com/a/5251383/281545 (edit: applied later for screens and installers - TODO: saves) Actually this is pretty bad - try renaming a screenshot with a filename containing for instance `:` - you will lose your screens - except if you want to have them as alternate data streams: http://stackoverflow.com/q/34097936/281545

Under #259. Fixes broken UI when an exception is thrown - the file would be left displayed as whatever>?.rar/jpg (>,? are invalid chars) - ofc the filename on disk was not changed. TODO: factor out common code Added a comment in balt and took wx stuff out of ScreenList.OnLabelEdited Shadowing warns (now that the code is visited)

Under #259, #243 (for WindowsError that would be convenient here but windoze only). Before as I state in 259 if the oldPath failed to be unlinked it would be actually duplicated in newPath

Under #259. From: http://stackoverflow.com/q/34097936/281545 This belongs to bolt or env - WIP.

Under #219, #259. Actually I filled 259 while working in renames for the installers tab - whatever I turn the eye to proves a can of worms. This branch makes installers renames _much_ faster and actually points to a general solution for the lag in installers operations - namely we need to avoid refreshes of mods and ini tabs as hell. Working on this I realized that renaming in Bash has several issues - exceptions thrown would leave the UI in an inconsistent state, filename restrictions were too loose (leading in the case of `:` to a serious bug, the file to be renamed would be made to an ADS, aka deleted as far as the user was concerned), 'file in use' would result in files being duplicated etc. The fixes here are very much a wip (have not looked at all at the save tab for instance) but at least screens and installers are pretty much fixed. Signed-off-by: MrD <the.ubik@gmail.com>

lojack5 · 2015-12-06T18:24:40Z

Not sure how to deal with this. Probably need some sort of "is valid name" check, disallowing things that windows rename dialog would normally disallow (even if : does have meaning, Windows Explorer forbids you from using it.)

Utumno · 2015-12-06T18:29:19Z

@lojack5: i've already dealt - for what was going on see: http://stackoverflow.com/q/34097936/281545.

Mopy/bash/basher/__init__.py: - Using super in InstallersList.OnBeginEditLabel - under #259 + event handling belongs to balt - Tiny optimization - calling bosh.dirs['installers'].list() once - adds virtually nothing to performance but adds a bit to readability methinks - No need for modInfosChanged variable - shadowing warns (file -> apath) - Using iteritems and renaming name to less generic, err, name Mopy/bash/bosh/__init__.py: - data pruning, clean -> clean_data_dir - delete_Refresh: deleted wrong comment - filterInstallables will now blow if passed in keys are not in InstallersData Mopy/bash/basher/installers_links.py: clean -> clean_data_dir, code style (indexing with True/False) Mopy/bash/brec.py: wrapping Some comments' edits

Made __setUI private - _gList must become private to UIList finally as a first step towards #255. Seems that one can't DnD while clicking on the icon so this was not needed - deleted now unused method SetDnD(). Took the opportunity to prune another use of _gList - the override was not needed (and slightly buggy, `if selected > 0:` is always true, even for empty list). See: http://wxpython-users.1045709.n5.nabble.com/how-to-find-onto-all-the-items-of-a-ListCtrl-td2355029.html for FindItem, replaced by the use of ListCtrl API Under renames (#259) check rename links.

Note small differences (parentheses, starting from 0) are squashed. I don't really like `and count < 1000:` but hey Under #259.

I don't like my additions to the CoSaves API (in general now that I look closer the Cosaves api needs refactoring). I also do not like I copy pasted name validation but factoring out common validation code proved a mess and I can't afford the time. However I needed to merge this as it fixes various cases of bad error handling. Under #259.

WIP merge finalizing the error handling fixups in renames (saves tab). I needed to merge the fixes but the API needs work: - factor out common code for valid filenames - hard ! - CoSaves API is tangled up - UIList_Rename overrides should be eliminated Under #259. Signed-off-by: MrD <the.ubik@gmail.com>

@Sharlikran

File patterns belong to respective classes as much as anything, if not more. Use class API instead of regexes and bush.game variables directly. Triggered by @Sharlikran's attempt to extend cosaves files extensions for fallout. For cosaves: - Added re.U, does not seem to harm - Scenarios with renaming/disabling and especially renaming a disabled save need investigation - we should not have 2 regular expressions but given that quicksaves and autosaves can not be disabled and renames is an open issue (#259) this is good enough for now (behavior has not changed). Note rename for bak files is broken (cosaves are not detected) Removed outdated comment on LO

Dropped refresh call - refreshes local save from ini which should not be needed plus FileInfos.rename should do the right thing - if not, patch that. Note FileInfos.rename uses env.shellMove as opposed to Path.moveTo - again should not make a difference. Note `fileInfo.madeBackup = False` in FileInfos.rename - under #292 Under #259.

Under #259, cosaves would not move along so broken - still WIP, I need to drop: if not newName.lower().endswith(bush.game.ess.ext): newName += bush.game.ess.ext

Wip merge addressing the details API (under #163 and #293) and adding UI enhancements (mainly selecting items after various operations (may have missed a couple), adding Counting Files progress in unpackToTemp and more work on saves renames - see #259). Took ages to get it right. Signed-off-by: MrD <the.ubik@gmail.com>

9d286e1 introduced a regression, namely hitting enter on "Move To" dialog would open the moved installer. Turned out that what triggered this was unbinding the OnShow event via the @balt.conversation in Installer_Move. --------------------- Mopy/bash/basher/installer_links.py --------------------- index 917a121..2d2337b 100644 @@ -616,3 +616,3 @@ class Installer_Move(_InstallerLink): - @balt.conversation + @balt.conversation ##: enter key is propagated to installer def Execute(self): Which send me to a goose chase trying to "fix" balt.conversation (notice the (wrong) comment above): def BindRefresh(self, bind=True, __event=wx.EVT_ACTIVATE): if bind: self.Unbind(__event) self.Bind(__event, self.RefreshData if bind else self._noop) if not bind: self.Bind(__event, self._noop) def _noop(self, event): pass # event.Skip or Veto won't do which did nothing to prevent the enter keystroke being propagated. What solved it was moving the enter handling in OnChar - explanation: unbinding RefreshData made the operation fast enough for Bash window to gain focus, so the Key Up event (on hitting enter in the move dialog) was caught by it and fired its event handler (open the installer) ! Applied the same cure to screnshots, got me rid of rename opening those (under #259). Yey ! This has tripped me too long - note that it was not always reproducible (depending on how fast I hit enter...) Fixed another regression introduced in same commit - passing an order greater that installers data length would move to last but keep the order to this large value. Finally b05821b made apparent a flaw in renaming - installer markers would all have their archive set to u'====' leading to: Traceback (most recent call last): File "bash\balt.py", line 2354, in __Execute self.Execute() File "bash\balt.py", line 1574, in _conversation_wrapper return func(*args, **kwargs) File "bash\basher\installer_links.py", line 638, in Execute self.window.SelectAndShowItem(GPath(current_archive.archive), focus=True) File "bash\balt.py", line 1969, in SelectAndShowItem self.SelectItem(item, deselectOthers=deselectOthers) File "bash\balt.py", line 1933, in SelectItem dex = self.GetIndex(item) File "bash\balt.py", line 2065, in GetIndex return self._gList.FindIndexOf(item) File "bash\balt.py", line 1552, in FindIndexOf return self.FindItemData(-1, self._item_itemId[item]) KeyError: bolt.Path(u'====') Fixed Signed-off-by: MrD <the.ubik@gmail.com>

See: http://forums.bethsoft.com/topic/1604190-wrye-bash-thread-110/page-3#entry25188212 - allow arbitrary characters in marker names - drop "extension" processing in marker names Under #259.

We should be calling SaveInfos#rename as I do here - the table row was not copied, comments would be lost - plus performance, directly messing with cosave extensions, unnecessary refresh and whatnot. Yak! I added reselecting the saves and dropped enabled link from Save_RenamePlayer - context menus are never displayed if selected is empty - more of this on next commit. Under #259 essentially.

Requested by @alt3rn1ty: http://forums.bethsoft.com/topic/1612572-wrye-bash-thread-112/#entry25268390 Turned out an interesting programming problem: http://stackoverflow.com/q/40737145/281545 Testing this I run across a bug - namely renaming screenshots to a number only would not work (and leave the UI in an inconsistent state, that is, only the selected screen would _appear_ renamed as event.Veto() was never reached): @@ -3018,3 +3017,3 @@ def OnLabelEdited(self, event): ext=self.__ext_group) - if not root: return + if not (root or numStr): return # allow for number only names selected = self.GetSelected() Threw in some changes in balt.UIList#validate_filename that are probably not needed, just this event.Veto() not being called irked me. Results in a used without being defined warning, left it as validate_filename does really need cleanup - separately check extension for starters. Ghost of #259.

Regression in disabling saves: Introduced in de878fb. Turns out rest of rename operations were calling refreshFile (except Save_Renumber) so it was not an issue, as a new fileInfo was created. Still this is an indication that we're in middle refactoring the FileInfos classes and we need to finish this up - have name become a property and always update abs_path instead - #336. Under #259 too - rename is still hacky (although much more centralized). Mopy/bash/basher/__init__.py: Wildcard needed those stars (introduced in f3038c6) Mopy/bash/bosh/__init__.py: Fixup in cached_lo_insert_after, None compares less than integers. Signed-off-by: MrD <the.ubik@gmail.com>

Under #259 for reselecting renamed screenshots and its details - installers remain. I dropped GetDetailsItem, self.panel.detailsPanel accesses should be revisited though.

Would do a bunch of crazy stuff if I tried anything (like adding text after the ==, deleting those etc). The installers rename is rename's black sheep - under #259.

Made apparent in 9428a03 but the roots were deeper. The traceback reported in #92 (comment) ... File "bash\bosh\__init__.py", line 3510, in _rename_operation FileInfos._rename_operation(self, oldName, newName) File "bash\bosh\__init__.py", line 2534, in _rename_operation fileInfo.abs_path = self.store_dir.join(newName) AttributeError: can't set attribute was due to the peculiar override rules of properties: http://stackoverflow.com/a/7020531/281545 But then the rename was broken for ghosts due to the newName being set to the ghost one. Solved at the root by finally removing this intentionally ugly piece of code (badly indented) from FileInfos#_rename_operation. I added todos for backups - under #292 WIP - I should drop tuples in _backup_infos Under #259

I had to remove __renaming_type to use new_name() - it made me uneasy since I wrote it (to replace existing logic) - double underscore usually means _revisit_ , very dangerous to leave it in there. Performance to get the selected items should be negligible - it's still ugly, we should find a way to absorb this into the Installer* classes - under #259. Note the ugliness of `to_rename and...`, due to the method being called in MEI where there are no selected items in the ui list. Using self.window.new_name in there allowed me to absorb the `bass.dirs['installers']` into createFromData where it belongs - not sure why we use projectName.root, dropped as I don't deal with Paths here - had to add GPath calls in new_name. From future: no need to call list() - directly use sorted(), some wrapping. Monkey patch for monitor external not detecting new project: https://bethesda.net/community/post/344453: Traceback (most recent call last): File "bash\balt.py", line 2490, in __Execute self.Execute() File "bash\balt.py", line 1602, in _conversation_wrapper return func(*args, **kwargs) File "bash\basher\installers_links.py", line 170, in Execute override=False) File "bash\bosh\bain.py", line 2297, in bain_install override) File "bash\bosh\bain.py", line 2222, in _install self.moveArchives(packages, len(self)) File "bash\bosh\bain.py", line 2114, in moveArchives new_ordered = self.sorted_values(moveList) File "bash\bosh\bain.py", line 2291, in sorted_values else: pairs = [self[k] for k in package_keys] File "bash\bolt.py", line 1291, in __getitem__ return self.data[key] KeyError: bolt.Path(u'Bodyslide-BookofUUNP-20170513') Monkey patch: @@ -160,2 +160,3 @@ def Execute(self): # Refresh Installers - so we can manipulate the InstallerProject item + self.iPanel.frameActivated = True # yak, refresh data directly here self.iPanel.ShowPanel() Monkey patch in the sense that we should call a targeted refresh, just adding the new project - done later. frameActivated is a smell but still needed.

Does it make sense to duplicate? Backup makes sense - if anyone ever is using this This is broken and unfinished but shows (possible) parts of the code to change. Instead of overwriting UILIst._new_name override SaveList.new_name. This code is hacky and cranky and full of wx and the copy/move infos APIs need revisiting - but d992340 makes it at least possible (we needed cosaves filenames for bak)

To reproduce: 1. Make sure Auto-Ghosting is on 2. File.. > New Mod 3. Switch to Installers tab You should get something like this: Traceback (most recent call last): File "bash\basher\__init__.py", line 3614, in OnShowPage refresh_target=load_order.using_ini_file()) File "bash\balt.py", line 911, in _conversation_wrapper return func(*args, **kwargs) File "bash\basher\__init__.py", line 2962, in ShowPanel scan_data_dir) File "bash\balt.py", line 911, in _conversation_wrapper return func(*args, **kwargs) File "bash\bosh\bain.py", line 1521, in _projects_walk_cache_wrapper return func(self, *args, **kwargs) File "bash\basher\__init__.py", line 3007, in _refresh_installers_if_needed do_refresh = self.listData.refreshTracked() File "bash\bosh\bain.py", line 2134, in refreshTracked self.data_sizeCrcDate[key] = (apath.size,apath.crc,apath.mtime) File "bash\bolt.py", line 629, in size return os.path.getsize(self._s) File "C:\Python27-32\lib\genericpath.py", line 57, in getsize return os.stat(filename).st_size WindowsError: [Error 2] The system cannot find the file specified: u'G:\\steam\\steamapps\\common\\Enderal\\Data\\New Mod.esp' This fixes it by enhancing our notify_external API to allow us to tell BAIN about renames. Of course, this isn't really the best place for it - once again comes down to copyTo/moveTo APIs. Ref # 336. <--- RRR Mopy/bash/basher/__init__.py: Threw in a fixup for a random crash I got but couldn't reproduce. Probably wx quirks. Mopy/bash/bosh/__init__.py: Some 'renames' renames :P

Utumno added the C-todo Category: TODO, specific item that needs to be accomplished in working towards a goal label Dec 1, 2015

Utumno added the A-wx Area: wxPython interactions (the gui package, balt.py and the basher package) label Dec 5, 2015

Utumno added this to the 307 milestone Dec 5, 2015

Utumno added a commit that referenced this issue Dec 5, 2015

Better filenames error checking:

8154f5b

Under #259. From: http://stackoverflow.com/q/34097936/281545 This belongs to bolt or env - WIP.

Utumno added C-bug Category: Bug, an acknowledged defect in the program and removed A-wx Area: wxPython interactions (the gui package, balt.py and the basher package) labels Feb 28, 2016

Utumno added a commit that referenced this issue Mar 5, 2016

Factoring common rename code to UIList:

d102c59

Note small differences (parentheses, starting from 0) are squashed. I don't really like `and count < 1000:` but hey Under #259.

Utumno self-assigned this Mar 5, 2016

Utumno added a commit that referenced this issue Apr 2, 2016

Disallowing renaming of quick/autosave bak:

325f3bc

Under #259, cosaves would not move along so broken - still WIP, I need to drop: if not newName.lower().endswith(bush.game.ess.ext): newName += bush.game.ess.ext

Utumno added a commit that referenced this issue Sep 11, 2016

Corrections in renaming markers:

4a5c1dd

See: http://forums.bethsoft.com/topic/1604190-wrye-bash-thread-110/page-3#entry25188212 - allow arbitrary characters in marker names - drop "extension" processing in marker names Under #259.

Utumno changed the title ~~Refactor renames~~ Renaming saves/screens/installers fixups Oct 5, 2016

Utumno removed the C-todo Category: TODO, specific item that needs to be accomplished in working towards a goal label Oct 5, 2016

This was referenced Oct 8, 2016

Update LOOT support for importing Bash Tags #299

Closed

Refactoring Backburner #300

Closed

Utumno closed this as completed in f739aba Oct 12, 2016

Utumno added a commit that referenced this issue Dec 11, 2016

Some more RefreshUI passing parameters in:

abfb04b

Under #259 for reselecting renamed screenshots and its details - installers remain. I dropped GetDetailsItem, self.panel.detailsPanel accesses should be revisited though.

Utumno added a commit that referenced this issue Feb 5, 2017

Fixups for rename in markers:

a71719a

Would do a bunch of crazy stuff if I tried anything (like adding text after the ==, deleting those etc). The installers rename is rename's black sheep - under #259.

This was referenced Feb 23, 2021

Renaming Fixups Round 2 #580

Open

Collaborators #400

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renaming saves/screens/installers fixups #259

Renaming saves/screens/installers fixups #259

Utumno commented Dec 1, 2015 •

edited

Loading

Utumno commented Dec 4, 2015

lojack5 commented Dec 6, 2015

Utumno commented Dec 6, 2015

Renaming saves/screens/installers fixups #259

Renaming saves/screens/installers fixups #259

Comments

Utumno commented Dec 1, 2015 • edited Loading

Utumno commented Dec 4, 2015

lojack5 commented Dec 6, 2015

Utumno commented Dec 6, 2015

Utumno commented Dec 1, 2015 •

edited

Loading