Created attachment 370929 [details] [review]
patch-1

Created attachment 370930 [details] [review]
patch-2

Created attachment 370936 [details] [review]
patch-3

Review of attachment 370936 [details] [review]:

OK

Review of attachment 370929 [details] [review]:

Would be nice to have colorizeRows I guess, anyway :-)

Review of attachment 370930 [details] [review]:

Apart from those "insignificant" points this looks good. Looks like you removed some unit tests but better have less tests but have them working than have broken tests fmpov!

Just fix that small remark and push I would say!

(Also I tested and it seems to work properly here :-))

::: debug-viewer/GstDebugViewer/Data.py
@@ -148,3 +149,3 @@
     LEVEL = "([A-Z]+)\s+"
     # "0x8165430 "
-    THREAD = r"(0x[0-9a-f]+)\s+"  # r"\((0x[0-9a-f]+) - "
+    THREAD = "(0x[0-9a-f]+)\s+"  # r"\((0x[0-9a-f]+) - "

Why did you remove the leading `r` heard, those are all regex and it is better to mark that.

::: debug-viewer/GstDebugViewer/Main.py
@@ +37,1 @@
     from GstDebugViewer import version

We should use same versionning as GStreamer here fmpov :-)

(In reply to Thibault Saunier from comment #6)
> Review of attachment 370930 [details] [review] [review]:
> 
> Apart from those "insignificant" points this looks good. Looks like you
> removed some unit tests but better have less tests but have them working
> than have broken tests fmpov!
> 
> Just fix that small remark and push I would say!
> 
> (Also I tested and it seems to work properly here :-))
> 
> ::: debug-viewer/GstDebugViewer/Data.py
> @@ -148,3 +149,3 @@
>      LEVEL = "([A-Z]+)\s+"
>      # "0x8165430 "
> -    THREAD = r"(0x[0-9a-f]+)\s+"  # r"\((0x[0-9a-f]+) - "
> +    THREAD = "(0x[0-9a-f]+)\s+"  # r"\((0x[0-9a-f]+) - "
> 
> Why did you remove the leading `r` heard, those are all regex and it is
> better to mark that.
> 

Unintended mistake indeed. Good catch!

> ::: debug-viewer/GstDebugViewer/Main.py
> @@ +37,1 @@
>      from GstDebugViewer import version
> 
> We should use same versionning as GStreamer here fmpov :-)

Yeah that makes sense. I'll bump the version to current git.
Thanks for the reviews!

About the tests, yeah I actually removed the ones related to the removed RangeFilterModel. It seems like it could be reimplemented but I didn't see the use-case for it, to be honest.

(In reply to Thibault Saunier from comment #6)
> 
> ::: debug-viewer/GstDebugViewer/Main.py
> @@ +37,1 @@
>      from GstDebugViewer import version
> 
> We should use same versionning as GStreamer here fmpov :-)

I wonder if it would be worth generating a python file from Meson for this? For now I'll push the patch as-is and open a follow-up bug to track this issue if that's ok with you.

Created attachment 370951 [details] [review]
debug-viewer: Dispatcher source ID clean-up

(In reply to Philippe Normand from comment #9)
> (In reply to Thibault Saunier from comment #6)
> > 
> > ::: debug-viewer/GstDebugViewer/Main.py
> > @@ +37,1 @@
> >      from GstDebugViewer import version
> > 
> > We should use same versionning as GStreamer here fmpov :-)
> 
> I wonder if it would be worth generating a python file from Meson for this?
> For now I'll push the patch as-is and open a follow-up bug to track this
> issue if that's ok with you.

I we should port it all to meson, this would start running the tests automatically on CI too (I will have a quick look, it should be *very* simple :-))

Comment on attachment 370951 [details] [review]
debug-viewer: Dispatcher source ID clean-up

Reviewed by Thibault :)