ncssar / radiolog

SAR radio log program
Other
13 stars 3 forks source link

hiding of certain team tab causes tracebacks and 'shifted' incorrect team tabs #702

Closed caver456 closed 6 months ago

caver456 commented 7 months ago

Noticed on NARROWS Nov 30 (OP2).

Several related issues, probably with the same root cause that the team whose tab was hidden was apparently actually removed from extTeamNameList.

caver456 commented 7 months ago

Looks like this is a case issue. Many team tab deletions had happened successfully before the tracebacks started. It was a particular attempted deletion that failed due to case mismatch. Here's the relevant transcript snippet - notice that the sequence of once-per-second line 3795 ValueErrors begin immediately afterwards:

100408:tab context menu requested: pos=PyQt5.QtCore.QPoint(229, 12)
100408:  i=4  tabRect=PyQt5.QtCore.QPoint(136, 34):PyQt5.QtCore.QPoint(236, 0)
100408:  extTeamName=z_TeamPcsoDrone  niceTeamName=Team PcsoDrone
100409:tab context menu requested: pos=PyQt5.QtCore.QPoint(228, 14)
100409:  i=4  tabRect=PyQt5.QtCore.QPoint(136, 34):PyQt5.QtCore.QPoint(236, 0)
100409:  extTeamName=z_TeamPcsoDrone  niceTeamName=Team PcsoDrone
100411:deleting team tab: teamName=Team PcsoDrone  extTeamName=z_TeamPcsoDrone  niceTeamName=Team Pcsodrone
100411:  teamNameList before deletion:['dummy', 'Team 207', 'Team 217', 'Team PcsoDrone', 'Teammarine 1', 'Teamdronerom 31', 'Teammarine 2Yc', 'Teamsar 40', 'Teamsar 33']
100411:  extTeamNameList before deletion:['spacerLeft', 'z_Team00207', 'z_Team00217', 'spacer1', 'z_TeamPcsoDrone', 'z_Teamdronerom00031', 'z_Teammarine00001', 'z_Teammarine002Yc', 'z_Teamsar00033', 'z_Teamsar00040']
100411:  i=4
Uncaught exception
Traceback (most recent call last):
  File "radiolog.py", line 5289, in tabContextMenu
  File "radiolog.py", line 5756, in deleteTeamTab
ValueError: list.remove(x): x not in list
Uncaught exception
Traceback (most recent call last):
  File "radiolog.py", line 3795, in updateTeamTimers
ValueError: 'z_TeamPcsoDrone' is not in list
Uncaught exception
Traceback (most recent call last):
  File "radiolog.py", line 3795, in updateTeamTimers
ValueError: 'z_TeamPcsoDrone' is not in list
Uncaught exception
Traceback (most recent call last):
  File "radiolog.py", line 3795, in updateTeamTimers
ValueError: 'z_TeamPcsoDrone' is not in list
...

Here's the code in question:

        rprint("deleting team tab: teamName="+str(teamName)+"  extTeamName="+str(extTeamName)+"  niceTeamName="+str(niceTeamName))
        rprint("  teamNameList before deletion:"+str(self.teamNameList))
        rprint("  extTeamNameList before deletion:"+str(self.extTeamNameList))
        if extTeamName in self.extTeamNameList: # pass through if trying to delete a tab that doesn't exist / has already been deleted
            i=self.extTeamNameList.index(extTeamName)
            rprint("  i="+str(i))
            self.extTeamNameList.remove(extTeamName)
            if not teamName.lower().startswith("spacer"):
                self.teamNameList.remove(niceTeamName) # this is line 5756
                del teamTimersDict[extTeamName]
                del teamCreatedTimeDict[extTeamName]

The case mismatch issue does show up in the transcript. Note this line: 100411:deleting team tab: teamName=Team PcsoDrone extTeamName=z_TeamPcsoDrone niceTeamName=Team Pcsodrone

Note on the error line in the code, niceTeamName ('Team Pcsodrone' - only T and P are caps) is being removed from a self.teamNameList which only contains 'Team PcsoDrone' (also has the D uppercase). So, the remove operation fails.

So, the seemingly easy fix would be to change 5756 to self.teamNameList.remove(teamName) (instead of niceTeamName)

Need to due some homework to make sure this is the right way to do it, and, to confirm that it also fixes the once-per-second 3795 errors.

caver456 commented 7 months ago

Attempts to duplicate this are interesting... see #694 - attempts to hide the tab that was created when a new entry was added from 'Team PcsoDrone' were successful, because everything was lowercase during the deletion clause. The uppercase D never made it into extTeamNameList, so, everything worked. Also showing the transcript around the time that the team was added:

150137:Accepted
150137:newEntry called with these values:
150137:['1501', 'FROM', 'Team PcsoDrone', 'DEPARTING IC', None, 'In Transit', 1701558066.0240748, None, None, None]
150137:new team: newTeamName=Team PcsoDrone extTeamName=z_TeamPcsodrone niceTeamName=Team Pcsodrone shortNiceTeamName=Pcsodrone
150137:extTeamNameList before sort:['dummy', 'z_TeamPcsodrone']
150137:grouped tabs:{'NCSOcmd': [], 'NCSO': [], 'NCPD': [], 'TPD': [], 'GVPD': [], 'CHP': [], 'Numbers': [], 'other': ['dummy', 'z_TeamPcsodrone']}
150137:extTeamNameList after sort:['spacerLeft', 'z_TeamPcsodrone']
150137:next available hotkey:1
150137:Accepted2
...
...
150233:deleting team tab: teamName=Team Pcsodrone  extTeamName=z_TeamPcsodrone  niceTeamName=Team Pcsodrone
150233:  teamNameList before deletion:['dummy', 'Team Pcsodrone']
150233:  extTeamNameList before deletion:['spacerLeft', 'z_TeamPcsodrone']
150233:  i=1
150233:  deleted proxyModelList index 1
150233:Freeing hotkey '1' which was used for callsign 'Team Pcsodrone'
150233:  extTeamNameList after delete: ['spacerLeft']
caver456 commented 7 months ago

This might be the difference: in the problem session, the team field text was all caps (following the word Team) when it was first added:

080121:Accepted
080121:newEntry called with these values:
080121:['0800', 'FROM', 'Team PCSO DRONE', 'DEPARTING IC', '49861  44170', 'In Transit', 1701360057.5646505, '100', '3066', '3914.0342|N|12115.8200|W']
080121:new team: newTeamName=Team PCSO DRONE extTeamName=z_TeamPcsoDrone niceTeamName=Team PcsoDrone shortNiceTeamName=PcsoDrone
080121:extTeamNameList before sort:['spacerLeft', 'z_Team00102', 'z_Team00104', 'z_Team00105', 'z_Team00106', 'z_Team00107', 'z_Team00109', 'z_Team00208', 'spacer1', 'z_TeamPcsoDrone']
080121:grouped tabs:{'NCSOcmd': [], 'NCSO': [], 'NCPD': [], 'TPD': [], 'GVPD': [], 'CHP': [], 'Numbers': ['z_Team00102', 'z_Team00104', 'z_Team00105', 'z_Team00106', 'z_Team00107', 'z_Team00109', 'z_Team00208'], 'other': ['z_TeamPcsoDrone']}
080121:extTeamNameList after sort:['spacerLeft', 'z_Team00102', 'z_Team00104', 'z_Team00105', 'z_Team00106', 'z_Team00107', 'z_Team00109', 'z_Team00208', 'spacer1', 'z_TeamPcsoDrone']
080121:next available hotkey:8
080121:Accepted2

Somehow this meant that the capital D was preserved in extTeamNameList.

caver456 commented 7 months ago

Ok, looks like all of these issues (all four bullets in the initial post) can be duplicated by adding a new entry with callsign field 'Team PCSO DRONE'.

image

All three of these issues begin upon attempted hide of that team tab (which doesn't actually hide the team tab):

So - need to figure out the right way to deal with these case sensitivity issues, once again.

caver456 commented 7 months ago

Make sure to look at #393, #406, #452, #453, #636, #503 (there may be others) since those also dealt with case sensitivity. Test any solution here with all the scenarios from those other issues.

caver456 commented 6 months ago

Started a spreadsheet to help track this down: https://docs.google.com/spreadsheets/d/1RnCxpp27ZbEdEz7_HhtwSVkK_d4IIsbwJFWj_iwoPnc/edit?usp=sharing

caver456 commented 6 months ago

deleteTeamTab is called from five places in the code. It's called twice with the extended team name (extTeamName), and with the appropriate second argument set to True; the other two times it's called with the niceTeamName, with no second argument. This should be handled correctly by deleteTeamTab. See #478.

caver456 commented 6 months ago

Looking at the log and the code in some more detail, it looks like getNiceTeamName was being called a second time on what was already the result of getNiceTeamName. The first call worked and preserved the uppercase D, but the second call failed to preserve the uppercase D:

inside tabContextMenu:

        rprint("tab context menu requested: pos="+str(pos))
##      menu.setStyleSheet("font-size:"+str(self.fontSize)+"pt")
        bar=self.ui.tabWidget.tabBar()
        pos-=bar.pos() # account for positional shift as set by stylesheet (when 'more' button is shown)
        i=bar.tabAt(pos) # returns -1 if not a valid tab
        rprint("  i="+str(i)+"  tabRect="+str(bar.tabRect(i).bottomLeft())+":"+str(bar.tabRect(i).topRight()))
        extTeamName=self.extTeamNameList[i]
        niceTeamName=getNiceTeamName(extTeamName)
        rprint("  extTeamName="+str(extTeamName)+"  niceTeamName="+str(niceTeamName))

Corresponding lines in the transcript:

100409:tab context menu requested: pos=PyQt5.QtCore.QPoint(228, 14)
100409:  i=4  tabRect=PyQt5.QtCore.QPoint(136, 34):PyQt5.QtCore.QPoint(236, 0)
100409:  extTeamName=z_TeamPcsoDrone  niceTeamName=Team PcsoDrone

Farther down in tabContextMenu, where the action is handled - notice the rprint line has been added since the time of the transcript, as part of #698, so there's no note 'deleteTeamTabAction clicked' in the transcript:

        elif action==deleteTeamTabAction:
            rprint('deleteTeamTabAction clicked')
            self.deleteTeamTab(niceTeamName)

Now for the first lines of deleteTeamTab:

    def deleteTeamTab(self,teamName,ext=False):
        # optional arg 'ext' if called with extTeamName
        # must also modify related lists to keep everything in sync
        extTeamName=getExtTeamName(teamName)
        if ext:
            extTeamName=teamName
        niceTeamName=getNiceTeamName(extTeamName)
        # 406: apply the same fix as #393:
        # if the new entry's extTeamName is a case-insensitive match for an
        #   existing extTeamName, use that already-existing extTeamName instead
        for existingExtTeamName in self.extTeamNameList:
            if extTeamName.lower()==existingExtTeamName.lower():
                extTeamName=existingExtTeamName
                break
#       self.extTeamNameList.sort()
        rprint("deleting team tab: teamName="+str(teamName)+"  extTeamName="+str(extTeamName)+"  niceTeamName="+str(niceTeamName))
        rprint("  teamNameList before deletion:"+str(self.teamNameList))
        rprint("  extTeamNameList before deletion:"+str(self.extTeamNameList))

Continuing with the next few lines of the transcript:

100411:deleting team tab: teamName=Team PcsoDrone  extTeamName=z_TeamPcsoDrone  niceTeamName=Team Pcsodrone
100411:  teamNameList before deletion:['dummy', 'Team 207', 'Team 217', 'Team PcsoDrone', 'Teammarine 1', 'Teamdronerom 31', 'Teammarine 2Yc', 'Teamsar 40', 'Teamsar 33']
100411:  extTeamNameList before deletion:['spacerLeft', 'z_Team00207', 'z_Team00217', 'spacer1', 'z_TeamPcsoDrone', 'z_Teamdronerom00031', 'z_Teammarine00001', 'z_Teammarine002Yc', 'z_Teamsar00033', 'z_Teamsar00040']
100411:  i=4
Uncaught exception
Traceback (most recent call last):
  File "radiolog.py", line 5289, in tabContextMenu
  File "radiolog.py", line 5756, in deleteTeamTab
ValueError: list.remove(x): x not in list

So - try enhancing getNiceTeamName to just return its argument, unmodified, if its argument doesn't start with z_. Checked the code to confirm that all calls to getNiceTeamName are done with an extended team name as the argument.

caver456 commented 6 months ago

That wasn't it:

065023:non-team-hotkey "f" pressed; calling openNewEntry
065023:openNewEntry called:key=f callsign=None formattedLocString=None fleet=None dev=None origLocString=None amendFlag=False amendRow=None isMostRecentForCallsign=False
065023:newEntryWidget __init__ called: formattedLocString=None fleet=None dev=None origLocString=None amendFlag=False amendRow=None isMostRecentForCallsign=False
065024:newEntryWidget.addTab called: labelText=0650  widget=<__main__.newEntryWidget object at 0x000002127DF67E30>
065024:inserting tab
065028:Accepted
065028:newEntry called with these values:
065028:['0650', 'FROM', 'Team PcDr', 'STARTING ASSIGNMENT', None, 'Working', 1702997423.9871728, None, None, None]
065028:getNiceTeamName called for z_TeamPcdr
065028:new team: newTeamName=Team PcDr extTeamName=z_TeamPcdr niceTeamName=Team Pcdr shortNiceTeamName=Pcdr

extTeamName was already generated without the uppercase D by the time getNiceTeamName was called for the first time.

newTeamName is the only one that preserves the uppercase D. Following it backwards through the call chain, it is taken directly from values[2] in newEntry - but is called 'niceTeamName' in local variables there and in newEntryProcessTeam, which is a bit misleading.

Basically, seems like there's not a great way to preserve camelCase in this manner. If there's a space, the second word is capitalized, which make sense, but without a space, everything is lowercased.

caver456 commented 6 months ago

Maybe getExtTeamName is the core issue:

064134:getExtTeamName called with argument "Team PcDr"
064134:  --> extTeamName="z_TeamPcdr"
064134:getExtTeamName called with argument "Team PC DR"
064134:  --> extTeamName="z_TeamPcDr"
064134:getExtTeamName called with argument "Team AB CD"
064134:  --> extTeamName="z_TeamAbCd"
...
064134:getExtTeamName called with argument "Team AbCd"
064134:  --> extTeamName="z_TeamAbcd"
064134:getExtTeamName called with argument "Team Pcdr"
064134:  --> extTeamName="z_TeamPcdr"

A focused solution might be to modify getExtTeamName to treat camelCase as different words, by inserting a space before each capital letter (after the first lowercase letter).

This wouldn't address any of the 'rule change' ideas in the spreadsheet, but, it would minimally fix this issue.

caver456 commented 6 months ago

This seems to have worked! Added these lines near the start of getExtTeamName (before any other modifications take place):

#702 - change camelCaseWords to camel Case Words
#  insert a space before every Uppercase letter that is preceded by a lowercase letter
teamName=re.sub(r'([a-z])([A-Z])',r'\1 \2',teamName)

This allows the initial call (whether it has a space or is already camelCase) to preserve case, as well as subsequent calls of the camelCased name to preserve case:

072325:getExtTeamName called with argument "Team Ab Cd"
072325:  --> extTeamName="z_TeamAbCd"
072325:getExtTeamName called with argument "Team PC DR"
072325:  --> extTeamName="z_TeamPcDr"
072325:getExtTeamName called with argument "Team PcAb"
072325:  --> extTeamName="z_TeamPcAb"
...
072325:getExtTeamName called with argument "Team AbCd"
072325:  --> extTeamName="z_TeamAbCd"
072325:getExtTeamName called with argument "Team PcAb"
072325:  --> extTeamName="z_TeamPcAb"
072325:getExtTeamName called with argument "Team PcDr"
072325:  --> extTeamName="z_TeamPcDr"

And, the final test - all of these tabs can be hidden without issue.

So - will close this issue with the commit and merge, but, should make a new issue for implementation of the proposed name rule changes shown in the spreadsheet.