Solve bug: Credits window not popping up #3909
Conversation
…popping out The variables from the tuple returned by icon_mapping.get() were in wrong order.
There was a problem hiding this comment.
Hey @KilianMaes, thanks for submitting this. Feedback follows, purely from reading the code as I haven't had a chance to test the branch yet (but will, after I post this).
| @@ -82,9 +82,9 @@ def update_model(self, filter=None, clear=True): | |||
| # Append type icon (PayPal, Kickstarter, Bitcoin, or Patreon) | |||
| item = QStandardItem() | |||
| for contrib in [n for n in self.icon_mapping if n in data["icons"]]: | |||
| (icon, tooltip) = self.icon_mapping.get(contrib, (None, None)) | |||
| (tooltip, icon) = self.icon_mapping.get(contrib, (None, None)) | |||
There was a problem hiding this comment.
You're quite right about that, good catch!
src/windows/models/credits_model.py
Outdated
| item.setIcon(icon) | ||
| item.setToolTip(tooltip) | ||
| item.setToolTip(_(tooltip)) |
There was a problem hiding this comment.
You don't want to retranslate here. The contents of self.icon_mapping are already translated (see lines 111-128) so tooltip will already contain a translated string.
There was a problem hiding this comment.
I didn't notice that, you're right. I changed this
src/windows/models/credits_model.py
Outdated
| @@ -56,7 +56,7 @@ def update_model(self, filter=None, clear=True): | |||
|
|
|||
| for person in self.credits_list: | |||
| # Get details of person | |||
| data = defaultdict("") | |||
| data = defaultdict(list) | |||
There was a problem hiding this comment.
Hmm. This is a tricky one. I never noticed that icons was a list of characters, in the JSON file, which would seem to support a list default.
Digression
If you look at the comprehension on 84:
for contrib in [n for n in self.icon_mapping if n in data["icons"]]I suspect data["icons"] was originally intended to be a string, and the icons would be single-character flags packed into that string. IOW, the idea was that data["icons"] would contain e.g. "knd" for a kickstarter contributor and Patreon supporter who's also a developer. The comprehension would match each of those characters, and it would display all of the appropriate icons.
Of course, whoever wrote this never got that to work right, because you can't set multiple icons for one cell of a data model. So then they added that s override for "multiple contributions". And then "icons" got turned into a list, because it's actually easier to export a list of characters than a packed string.
But the code also contains a lot of calls like data["name"].lower(), which would break on a list item. So, I suspect this should actually be defaultdict(str). Which will work for data["icons"] as well, since an empty string is still compatible with the in usage in the comprehension.
| data = defaultdict(list) | |
| data = defaultdict(str) |
Just for fun, I notice now that there are also several rows in the JSON with a "t" in the icons list. What does "t" mean? Search me, it's not defined anywhere in the code. I suspect they might be unused translator credits. Those ended up getting pulled from the translation file itself, instead.
There was a problem hiding this comment.
Heh. Oh, the defaultdict was me. (As was the icon_mapping, so the bug here is definitely my fault.)
Well, then I can say confidently that it should be defaultdict(str), because the previous code (which was convoluted and verbose, but basically equivalent) definitely treated the data values as strings. I just misunderstood how to create a defaultdict, apparently.
There was a problem hiding this comment.
Ok I replaced the "list" by "str" in the PR. Everythings seems to work fine. I didn't notice the "t" before you mention it so I have no idea what it stands for...
Two minor changes were made based on ferdnyc's comment, see PR OpenShot#3909.
|
Thanks for the feedback, @ferdnyc . I made the changes in the two lines you mentioned, I hope it's ok now |
|
@KilianMaes Sorry for the delay! I recently upgraded to Fedora 33, and since then OpenShot has been crashing on startup when I try to run it from the development tree! Haven't figured out what the issue is there, yet. I had to use a VM to test this. I can confirm that this fixes the issue with the credits window not showing up. The filter field is COMPLETELY broken, I notice — anything you type in there causes a few lines to be displayed from the table, which have no relationship to what you typed in. So, I'll have to debug that next. But it's clearly unrelated to your fix, which is the only reason we can even see the problem. Thanks again, merging! |
The variables from the tuple returned by icon_mapping.get() were in wrong order (+ minor change). Could you verify that this PR solves the bug? I'm not totally sure about the expected window that should pop up because I never saw it before solving this issue.
Fixes #3908