Fix crash & other LanguageTokenField issues #4074
Conversation
|
Pulled PR and tested. I was not able to reproduce the crash. Great work tracking that down! I did find the following problem that is not fixed. We could treat this as a separate issue. Type "Fr" and pick the entry for "fro" as shown here: After pressing return the result contains "french" as well as the ISO code: It is stored in preferences that way: low-batt@gag ~$ defaults read com.colliderli.iina subLang
french,froAnother thing I would change is to sort the list shown to the user. In the first screenshot I would have expected all the French language entries to be together before the entry for Friulian. |
svobs
force-pushed
the
pr-language-token-crash
branch
from
c9a3488
to
8f3631e
Compare
|
I had to rewrite the whole thing again. The double entry you pointed out was difficult to work around but I think I've got a workable solution. Some context:
The problem occurred for language To work around this, I was able to tell it to use newline delimiters instead of comma delimiters. This fixed the double token creation problem, but it broke copy & paste. The newlines would get stripped out during copy; and paste would only work if they knew to first copy with one lang code per line. Worst of all, it broke the prefs, which have been storing the list as CSV. So I was able to fix the new problems by adding extra code in the right places to translate from newline-separated-values to comma-separated-values, and back again. The values it saves to the prefs should almost always be language codes, not language descriptions, so using CSV there shouldn't be an issue. But just in case, I made it first replace any comma it found in each token with a semicolon before saving in the prefs. |
|
Forgot to mention that I also implemented sorting. It will re-sort as soon as a new token is created, and there is no animation to show the tokens moving around, so hopefully users will be able to figure out what's happening. I considered trying to sort the prefs without also sorting the displayed values, but it looked that that would have introduced a lot of tricky edge cases which I wasn't willing to deal with. |
svobs
force-pushed
the
pr-language-token-crash
branch
3 times, most recently
from
89afad4
to
b99ddbd
Compare
|
OK, it looks like the infinite loop is back. EDIT: OK. Fixed again. Yeesh. |
svobs
force-pushed
the
pr-language-token-crash
branch
2 times, most recently
from
5e57744
to
fe345f0
Compare
|
I am no longer able to reproduce the problem with storing On this:
The new code does not sort the entries displayed to the user? Instead it is sorting the codes in the stored preference value? I believe that is wrong as the order of codes in the preference represents the user's specified priority order in which IINA should look for subtitle languages. I should be able to set the preference to The sort order I was concerned about can be shown by this screenshot: The user must hunt around or type more characters to find all the different french codes. Also of note in that screenshot is that the list is not quite wide enough and the code for Displaying the list to choose from in sorted order is not critical. The user can type more characters to narrow down the list. Just a nice to have. So far I've not been able to reproduce this again. I think I had the preferences window open with the list being displayed and then selected Crash Report:<\details> |
svobs
force-pushed
the
pr-language-token-crash
branch
from
fe345f0
to
7f2a88f
Compare
|
Oh that makes more sense 🙃 It was easy enough to sort the language entries which show up in the auto-complete popup. However, I wasn't able to find a handle for the auto-complete popup itself anywhere, so I think we may have to live with the current width. From reading some forum posts, it looks like it is probably creating a child window, but there's just no hint about where it's doing that. If we wanted to customize it, we might be better off disabling the built-in autocomplete and just coding our own. (It might be a good long-term strategy to move away from NSTokenField in general. It's not efficient, and is very temperamental. Future support for it also seems dubious - Apple has deleted the example code for it, and there is at least one glaring bug in the API that has never been fixed.)
I wasn't able to reproduce it either. I agree - the stack trace seems to support your conclusion. Even without the infinite loop, the Maybe it would be possible to reduce the likelihood of this happening by adding some small delay after all the windows are closed, and before IINA terminates. Although...I don't think we are actually closing the |
|
I pulled the latest commit and tested. I only found one problem. The user should be able to drag the codes around to change the priority order. That is now half working. I can drag the code at the end of the list to the left to the front of the list. But if I then try and drag that code from the front of the list right and put it back at the end of the list, I can move the token to the end of the list and it looks like it is going to work, but when I let go of the mouse button the code is not moved from the front of the list. I did try hard to trigger a constraint error crash. It passed all my tests. |
svobs
force-pushed
the
pr-language-token-crash
branch
from
7f2a88f
to
5e3c790
Compare
Surprisingly hard problem, given that I'm trying to do duplicate removal too. For drag & drop like this, even though it should be a "move", it's sending 2 notifications, as though it's first copying & pasting, and then doing a delete as a second transaction. This, combined with the fact that I've begun doing the updates tracking in code, rather than trying to rely on the I came up with a quick & dirty diff on two CSV lists (the prev and new field entries), to guess which token was being copied, and remove that. It seems to work pretty well, except that because it's setting the If that's too much of a compromise, I can remove duplicate detection entirely. Too many unknowns to figure this out more completely. I'm cuckoo from Cocoa Dev! |
|
I'm confused by the explanation, as in I'm not understanding why dragging from the back to the front worked, and yet the other way didn't. No matter. I pulled the latest commit and could not break it. All seems good, but there are two new compilation warnings that need to be addressed: It is really good to get issue #3505 fixed. Users have been reporting this for a long time. I think IINA should be putting a priority on making sure subtitle support is fully working as the ability to download subtitles is a very useful special feature that IINA provides. There still are a number of problems in this area of IINA. Fixing the UI for this preference is an important step in fixing subtitle support. |
…ding from saved prefs. Disallow duplicate language entries. Sort autocomplete entries. Trim language tokens for whitespace & make lowercase.
svobs
force-pushed
the
pr-language-token-crash
branch
from
5e3c790
to
d2ecd93
Compare
It's because of the relatively sloppy way that
What should be one notification instead shows up as two. Violates API best practices. But it's also not considered good practice for us to wait some arbitrary amount of time and try to guess which notification is the "final" one. The code I wrote was already taking the first notification and treating it as the correct one, but it noticed it contained a duplicate token. And so the de-duplication code was too simple: it went through the string and saved the first occurrence of each token. By saving only the first occurrence, my code was setting So I had to come up with a solution which would look at the strings from (1) and (3) above, and try to figure out which But anyway... Fixed the warning. The latest version of XCode has a few new bugs (I'm back on an Intel Mac for now, if that matters). One of them is that warnings sometimes don't appear until I close the file and reopen it. ¯_(ツ)_/¯
There are big pieces of IINA which I don't use extensively, and although I generally check the newer issues as they're filed, I haven't waded through the backlog much; so I was unaware that such a prominent crasher existed. If there are other issues which you see as high-priority I'd be happy to take a look. Lastly - this is a tangent but have you seen OpenAI's Whisper? I've been playing with it and it's incredible. At least for English. But makes me wonder whether downloading subtitles will even be necessary soon, if they can just be generated on-the-fly. |
|
Thanks for the detailed explanation. All clear to me now. Just finished testing on both macOS 13.0.1 and 10.15.7, all good. There is one other crash I have been worried about. I'm pretty sure no one is looking into this one. It is another constraints related crash, but much more serious as IINA is crashing while animating the resizing of the main window. It was reported under macOS 11.5.2. Big Sur had issues. I don't know if the problem was tied to macOS 11. It did not reproduce for me. I did not have time to do a lot of testing. It is issue #3615. Let me know if you see something I didn't. By the way are you using git commit --amend? The usual way I pull PR updates is failing: low-batt@gag iina (pr4074)$ git pull origin pull/4074/head
remote: Enumerating objects: 4, done.
remote: Counting objects: 100% (4/4), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 4 (delta 2), reused 4 (delta 2), pack-reused 0
Unpacking objects: 100% (4/4), 4.23 KiB | 866.00 KiB/s, done.
From github.com:iina/iina
* branch refs/pull/4074/head -> FETCH_HEAD
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint:
hint: git config pull.rebase false # merge
hint: git config pull.rebase true # rebase
hint: git config pull.ff only # fast-forward only
hint:
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.
low-batt@gag iina (pr4074)$ I end up just starting from scratch to get the latest commit. |
|
I'll take a look at #3615.
Nope :) - I've been doing |
|
Definitely do not use --amend. That is considered bad to do for a published commit. I think what you want to do is to squash your commits. |
|
There have been several requests for live subtitle generation. Issue #3867 is one of them. You will see I mentioned DeepSpeech as an example of a speech to text engine. This is definitely an area to investigate. |
Description:
This was a really tricky one.
The crash log wasn't helpful at all, and the problem wasn't at all related to constraints, but it was accurate in hinting about an infinite loop which AppKit was detecting and killing. By placing log statements all over the code and running it, I was able to determine that the infinite loop was coming from the
LanguageTokenFieldclass.For some reason, the
tokenField(displayStringForRepresentedObject)andtokenField(representedObjectForEditing)methods were getting called endlessly and at a ratio of about 5:1. I know that AppKit is kind of messy and tends to call methods a lot more than it needs to, so I wasn't really concerned by all thedisplayStringForRepresentedObjectcalls, but the smoking gun was the repeatedrepresentedObjectForEditingcalls, which is where AppKit asks the code what the represented object ("token") should be for the text in the text field. The fact that it was calling it over and over suggested that it either wasn't happy with the result it was getting, or thought it hadn't called it. From my experience doing object-mapping in the past, and seeing that a new Token() was being returned each time, I had a hunch that AppKit wasn't able to establish an identity for the Token objects it was getting back - and that turned out to be correct. I was able to get it to stop crashing by adding an override ofisEqual()to theTokenclass which used itscodefield to check whether one token was identical to another. (SinceisEqualwas not overridden before, the system would default to comparing object references, but since a newTokenwas being returned each time, that would be seen as a new object.)I don't know why it's doing what it's doing; it's AppKit and the source code isn't available, but during my research I found a commenter who seemed to sum it up well:
NSTokenFielddoesn't really like to be customized. As far as I know, there's nothing documented about the need for a returned token to guarantee identity, and that is a glaring omission on Apple's part.During the course of this investigation I tore the
LanguageTokenFieldapart and pieced it back together again. There were a number of things which didn't seem like good ideas, like overridingstringValueand the strange use ofLayoutManagerlooking for a Unicode attachment char as a way of figuring out whether the user finished typing. I refactored to get rid of the former...but the latter turned out to be essential, because it was catching cases where the standard events were not getting fired.But there were a few other issues which I discovered and did fix in this update:
controlTextDidChangewasn't getting called. Fixed by adding more listeners.Tokenobject was getting itscontentfield set to the same value as itscodefield. So double-clicking a token to edit it didn't show the full name of the language as it did before. I managed to remove theTokenclass entirely and replace it with language list lookups. Less code, so less chance for bugs, and still about as fast. Also removes the need to do a customisEqual, because now it's just comparingStrings.fraandFrato be considered two different languages. So all the entries, including user entries which aren't recognized in the language dict, are now converted to lowercase before being stored in the prefs and before being checked for equality.Let me know if that last point was an incorrect assumption to make and I can remove that check.