Small Title Editor fixes

[ferdnyc]

• Update timer:

  • Made the update timer single-shot, so there's no need to stop() it explicitly
  • Parented the timer to the class, so it'll be cleaned up by Qt when the dialog closes
  • Moved the super().__init__() call before the timer creation (required for parenting)

• QFont.setPixelSize() only takes int arguments (and throws a ValueError on Python 3.10+ if you pass it a float)

• Removed a useless binding of the _ translation function where it isn't needed.

[JacksonRG]

LGTM

[JacksonRG]

Just curious. You said this causes Qt to clean up the timer when the dialog closes. So as it was, would update_timer hang out as a in the class until it got over written by the next instance?

[ferdnyc]

@JacksonRG Oh, no, what would happen is that on shutdown (or other destruction of the parent object), the timer would potentially still be running and generating signals, which would become undeliverable once their target slots were deleted, leading to crashes on exit.

As long as you were careful to always stop the timer before the object is destroyed, and possibly call processEvents() to clear the signal queue, everything worked fine. If you weren't careful enough, you'd find out eventually because sometimes — not always, just to make things fun — exiting the application would lead to crashy-crashy.

...Parenting the timers means you don't have to fuck around with any of that AT ALL. You just call .deleteLater() when you want to destroy stuff "by hand", or let Qt do it automatically on exit.

Qt, through the magic of .deleteLater(), will ensure that the timer is stopped and all of its signals processed (along with the signals from any other children, like all of the child widgets owned by a widget class's layout) before it actually deletes the parent object.

That's another reason why I dislike code emitting other classes' signals so much. If a class only emits its own local signals, if everything is parented properly, and if lifetimes are managed using .deleteLater() to destroy objects, then Qt ensures proper ordering so that no references are left dangling that lead to crashes.

But if classes are constantly making direct reference to resources in other classes, then the objects' interdependencies become a hopelessly tangled spaghetti that makes setup and teardown both complicated and fragile. (Hence, for instance, my #4697)

[ferdnyc]

(The other reason is testing. The reason we can't unit-test any of the OpenShot code has nothing to do with it being GUI code, and everything to do with the fact that nearly all of its classes have methods that can't be called — and in many cases, the classes themselves can't even be instantiated — because they blindly reference instance variables in other, unrelated classes.
`

# (1) Whenever a class method does this...
get_app().window.someSignal.emit()
# (2) or this...
get_app().window.timeline_sync.timeline.Something()
# (3) or this...
self.window.preview_thread.current_frame
# (4) or this..
win.propertyTableView.select_frame(win.preview_thread.player.Position())

It's assuming not only that its parent application is an instance of classes.app.OpenShotApp (not any other generic QApplication or QCoreApplication instance), but also that it must have already instantiated...

1. Its QMainWindow instance, which must be a windows.main_window.MainWindow or it won't have someSignal defined
2. The QMainWindow from windows.main_window.MainWindow, plus its classes.timeline_sync.TimelineSync instance, which must have set up its local timeline instance of openshot.Timeline
3. The QMainWindow from windows.main_window.MainWindow, which must have instantiated windows.preview_thread.PreviewParent and run its Init() method (which requires instances of classes.timeline_sync.Timeline and windows.video_widget.VideoWidget), and that must have started a QThread containing a windows.preview_thread.PreviewWorker instance, which then must have been assigned to the preview_thread variable local to the windows.main_window.MainWindow instance
4. The QMainWindow from windows.main_window.MainWindow, which must have instantiated windows.views.properties_tableview.PropertiesView, plus everything from item 3, where the windows.preview_thread.PreviewWorker instance must also have instantiated an openshot.QtPlayer instance.

...Code written like this is impossibly fragile. It's built on a house of cards, and far too easy to accidentally topple over. The moment anything happens to change the order in which objects are created or destroyed, even ones that seem totally unrelated (which can happen if the version of Qt/PyQt/python changes, or it's run on a system with a different CPU topology or core count) the whole thing collapses.

But if, OTOH, classes are written to be self-contained and rely only on their local data, emitting signals to broadcast changes and relying on their local slots to receive external data, then every object can exist in isolation and testing becomes a breeze. You can unit-test any part of the class by simply creating an instance (which shouldn't have to know or care about whether any particular objects from any other classes exist or not), then creating signal connections to its slots and delivering the inputs it would expect from the "real" source. The fact that those inputs are coming from a mocked class, not an instance of the real source, ideally would be invisible to the tested class.

Admittedly, that's not always simple to do, and very hard to do perfectly — some interdependency is a given. Writing every call inside a try block, or existence-checking every external variable you refer to, is no way to code.

I've managed to eliminate most of the external dependencies in the classes modules, so that they can be instantiated as a group even when none of the GUI objects exist. But they're still heavily interdependent with each other, something I've accepted as the lesser evil.

Access to external data nearly always requires some interdependency. Only one object can hold that data, and if it's supposed to be unique then copying it requires lots of annoying synchronization. So, centralizing those things around the OpenShotApp instance at least consolidates the dependencies. It also manages lifetimes properly, since the app instance will outlive all other objects.

(That's why, for example, everything that needs to access settings goes directly — via the OpenShotApp.get_settings() accessor — to the app's local settings instance, rather than directly using classes.settings.Settings and possibly creating duplicates.)

But the code in windows doesn't even try. Every windows.** class implicitly requires not only the existence of an instance of every other windows.** class, but most of them require an instance with a specific state. Most of them, in fact, you can't even instantiate outside of those very specific conditions. (Doing lots of complicated stuff in __init__() is just the worst, from the perspectives of code isolation, separation of concerns, and unit testing. Setting up instances of a class should be quick and cheap, and those objects should be as lightweight and disposable as possible, with the absolute minimum of external dependencies.)

[ferdnyc]

@JacksonRG I realized that, for all the verbiage I spewed there, I never directly answered your question. Having an unparented timer in a dialog class can bite you in one of two ways:

1. If the application is terminated while the dialog is open, the dialog might get destroyed while the timer is still running and still has unprocessed signals waiting on the event loop, leading to crashy-crashy.

2. If the timer is still running when all references to the dialog are deleted, then which one gets destroyed first is not necessarily guaranteed.

   (Destruction of the timer before the dialog should be guaranteed on the Python side, I believe, since the dialog instance holds the last reference to the timer. But that's only on the Python side.)

   Both of those classes are also backed with instances of their respective C++ classes, and their interdependence is invisible there. The only way the C++ QTimer instance can know that it's dependent on the C++ QDialog instance is to set the dialog as its parent().

[ferdnyc]

Merging #4738 reminded me that I should get this one in too, since it also contains a fix for an implicit float -> int narrowing, which are no longer permitted in Python 3.10+.