Replace deprecated GtkActionEntry with XfceGtkActionEntry

Alexander Schwinn @alexxcons said:

This change will as well fix the following bugs: Bug #14026

Alexander Schwinn @alexxcons said:

Tasks:

finish check for mem-leaks.

finished / fixed

Known Bugs:

When right click a folder in bulk renamer: (thunar:5481): thunar-CRITICAL **: 21:19:53.015: thunar_window_has_shortcut_sidepane: assertion '(((extension ({ GTypeInstance __inst = (GTypeInstance) ((window)); GType __t = ((thunar_window_get_type ())); gboolean __r; if (!__inst) __r = (0); else if (__inst->g_class && __inst->g_class->g_type == __t) __r = (!(0)); else __r = g_type_check_instance_is_a (__inst, __t); __r; }))))' failed Traceback (most recent call last):

fixed

Next TODO: re-write git history in a meaningful way. Try to keep thunar runable on each commit

Alexander Schwinn @alexxcons said:

Found a bug for the WIP branch: When doing DnD, thunar seems to enter a endless loop (high CPU + freeze) ... investigating that.

Most recent WIP branch, based on most recent master (Tidyup history is WIP): https://github.com/alexxcons/thunar/tree/ReplaceGtkAction16

Alexander Schwinn @alexxcons said:

(In reply to alexxcons from comment 3)

Found a bug for the WIP branch: When doing DnD, thunar seems to enter a endless loop (high CPU + freeze) ... investigating that.

Fixed in https://github.com/alexxcons/thunar/tree/ReplaceGtkAction17

Alexander Schwinn @alexxcons said:

Most recent branch, which partly has some git history: https://github.com/alexxcons/thunar/tree/ReplaceGtkAction41

Found another bug: Keyboard navigation (arrow keys) on window menu do not work. (They do work if the related menu got created once before)

Reuben Green @reubengreen73 said:

Hi Alex, I've found a (minor) bug on ReplaceGtkAction41

If you have two tabs open with different view types, then the state of the view type radio buttons does not get updated when switching tabs. So to reproduce, open a new tab, change the view type to something different, then change back to your original tab; the original tab will still have the same type of view as before, but the radio buttons will have the state of the second tab.

Hopefully we'll have per directory view settings soon (it's taken me a lot longer than I though to grok the thunar internals, but hopefully I get it now!)

Thanks!

Alexander Schwinn @alexxcons said:

Thanks for reporting Reuben! I can reproduce it ... I'll look into it.

Alexander Schwinn @alexxcons said:

(In reply to Reuben Green from comment 6)

Hi Alex, I've found a (minor) bug on ReplaceGtkAction41

Fixed now in ReplaceGtkAction42 (as well switched back to old-style commit message and bumped xfce4ui version to min requirement)

Pending bugs: Keyboard navigation (arrow keys) on window menu do not work. (They do work if the related menu got created once before)

Reuben Green @reubengreen73 said:

Hi Alex. So here is a bug that I've found on ReplaceGtkAction42.

To reproduce: change the view type via the radio buttons then press Ctrl-A to select all

Then the following warnings happen

(thunar:17200): GLib-GObject-WARNING **: 14:13:52.340: invalid unclassed pointer in cast to 'ThunarStandardView'

(thunar:17200): thunar-CRITICAL **: 14:16:33.376: thunar_standard_view_select_all_files: assertion '(((extension ({ GTypeInstance __inst = (GTypeInstance) ((standard_view)); GType __t = ((thunar_standard_view_get_type ())); gboolean __r; if (!__inst) __r = (0); else if (__inst->g_class && __inst->g_class->g_type == __t) __r = (!(0)); else __r = g_type_check_instance_is_a (__inst, __t); __r; }))))' failed

I think on some systems this causes a segfault, see comment 66 in bug #3521 (although that was with a patch of mine applied, but it seems to be the same bug).

I have taken backtraces of these warnings (via G_DEBUG=fatal-warnings) and I will attach them here.

Thanks!

Reuben Green @reubengreen73 uploaded an attachment:

Attachment 9786, "First backtrace":
bt1

Reuben Green @reubengreen73 uploaded an attachment:

Attachment 9787, "second backtrace":
bt2

Alexander Schwinn @alexxcons said:

(In reply to Reuben Green from comment 9)

Hi Alex. So here is a bug that I've found on ReplaceGtkAction42 ...

Thanks alot for the research Reuben ! I can reproduce the bug.

"thunar_standard_view_append_accelerators" will register to the accel key using a specific view as object. Even when I change the code to do so in thunar_window_action_view_changed, it looks like to object used to pass to the callback is not updated when the view is changed, so I guess the method will be called with a view which does not exist any more.

I'll check if "gtk_accel_group_disconnect" does the trick, so that the new view can register for an accel path ... or worst case, I have to move all the related Actions into thunar-window and call select_all_files there .. possibly throug thunar -view.

Reuben Green @reubengreen73 submitted a patch:

Another bug, but this time I have a patch for it (patch is against ReplaceGtkAction45).

The bug is that the default view set via the preferences dialog is not respected, and instead the last view is always used. To reproduce, set the default view to eg details view, then close thunar while viewing a directory in eg icon view. When you next start thunar, you'll be in icon view not details view.

The bug is caused by the call to thunar_window_action_view_changed in thunar_window_set_current_directory having window->view_type as an argument instead of the function's type variable.

The first commit in the patch fixes this, and also adds some necessary calls in thunar_window_init to ensure that the necessary types are all registered. If you just correct "window->view_type" to "type" in thunar_window_set_current_directory without adding the g_type_ensure calls, you can get segfaults (eg if you do the reproducer steps above).

The second commit removes all the last-view related code in thunar_window_init, as I think the previous commit makes it redundant, and after some quick testing it seems that removing it has no bad effects (but I did not test it very much).

(I found this while working on bug #3521, which is proving to be much more complicated than I thought, but it is teaching me lots about the thunar code and using gtk/glib, so it's good!)

Patch 9807, "use defalut view type":
type_fix.patch

Alexander Schwinn @alexxcons said:

(In reply to Reuben Green from comment 13)

Created attachment 9807 [details] use defalut view type

Thanks Reuben, though something is wrong there:

not setting "default-view" will just crash thunar
last-view seems to be not used any more .. whenever I start thunar, I get icon-view (however could be me doing something wrong .. I see the same when using thunar-master)

If I get it correctly, the idea is to use "last-view", if available (like for all other last_xxx items) For some reason I dont fully understand, there is as well "default-view" .. maybe for the first thunar startup ? So if "last-view" is not available, I think we should fallback to "default-view". (That I missed to implement) If that is as well not available, we should fallback to e.g. "ThunarIconView".

You share that idea of how it should work ? Or possibly I am wrong there ?

Reuben Green @reubengreen73 said:

(In reply to alexxcons from comment 14)

not setting "default-view" will just crash thunar

I tried removing the "default-view" setting via xconf, but this does not cause a crash for me. What are your steps to make this crash?

So I think the intended behaviour here is as follows. The default view (set via the preferences dialog) is the view type thunar should use when making a new window (so for example every time thunar starts up). You can set the default view to any of icon view, compact view, details view, or last active view. If you set it to last active view, then thunar will use the last view type (from the "last-view" property) for the tab in the new window. This behaviour is how it works currently in master.

As far as I can tell from some testing, my patch makes thunar behave as I described above (and thus in the same way as master). I guess we should keep the behaviour as it has been up to now, unless we can think of a better way?

Thanks!

Alexander Schwinn @alexxcons said:

(In reply to Reuben Green from comment 15)

(In reply to alexxcons from comment 14)

not setting "default-view" will just crash thunar I tried removing the "default-view" setting via xconf, but this does not cause a crash for me. What are your steps to make this crash?

I used xfce4-settings-editor to either put "empty string" or some random string into "default-view" ... no problem, I can further debug that and provide a fallback.

So I think the intended behaviour here is as follows. The default view (set via the preferences dialog)

Uh, damn .. I overlooked that setting in the preferences dialog :) ok, now everything makes sense to me :P Sorry for beeing a bit confused .... yes I totally agree that we should keep that functionallity :)

Thank salot for your patches, just pushed them to 'ReplaceGtkAction45' !

I as well pushed two more fixes to 'ReplaceGtkAction45'. And I pushed a fix to the master branch of libxfce4ui (double registration of accelerators, causing the crash above), which is now required when building #45 (closed).

The following is now fixed:

Crash when switching view and than pressing <Primary>A (select all)
Keyboard navigation (arrow keys) on window menu do work now

Pending bugs:

Shortcuts for UCAs do not work / are not shown (that used to work at some previous iteration .. have to sort it out)
Shortcuts without menu binding do not work (e.g. cycle tabs) .. have to be added manuall. Consequence of recent changes in libxfce4ui
crash if xfconf value "default-view" is empty

Alexander Schwinn @alexxcons said:

Pending bugs:

crash while doing keyboard navigation in window-menu: crash (F10 , up, left, right --> crash) .. as well often freeze on up/down

... meh, my hope was that bugs would get less over time :)

Reuben Green @reubengreen73 said:

(In reply to alexxcons from comment 16)

(In reply to Reuben Green from comment 15)

(In reply to alexxcons from comment 14)

not setting "default-view" will just crash thunar I tried removing the "default-view" setting via xconf, but this does not cause a crash for me. What are your steps to make this crash? I used xfce4-settings-editor to either put "empty string" or some random string into "default-view" ... no problem, I can further debug that and provide a fallback.

Right, I can get this crash now, and I see why it's happening. I think I can fix it fairly easily - I'll try to get a patch up soon.

Reuben Green @reubengreen73 submitted a patch:

So here is a patch (to go over my previous patch) to prevent thunar from crashing when the "default-view" or "last-view" properties have bad values (eg empty string, random nonsense). I've given it a quick test and it seems to work.

Patch 9813, "prevent crashes when properties have junk values":
second-fix.patch

Alexander Schwinn @alexxcons said:

(In reply to Reuben Green from comment 19)

Created attachment 9813 [details] prevent crashes when properties have junk values

So here is a patch (to go over my previous patch) to prevent thunar from crashing when the "default-view" or "last-view" properties have bad values (eg empty string, random nonsense). I've given it a quick test and it seems to work.

Thanks Reuben, works fine now for me! Pushed it to #45 (closed). Hope I have time to fix the other pending bugs soon.

Pending bugs:

Shortcuts for UCAs do not work / are not shown (that used to work at some previous iteration .. have to sort it out)
Shortcuts without menu binding do not work (e.g. cycle tabs) .. have to be added manuall. Consequence of recent changes in libxfce4ui
crash while doing keyboard navigation in window-menu: crash (F10 , up, left, right --> crash) .. as well often freeze on up/down

Replace deprecated GtkActionEntry with XfceGtkActionEntry

Submitted by Alexander Schwinn `@alexxcons`

Description

Designs

Child items 0

Activity

Replace deprecated GtkActionEntry with XfceGtkActionEntry

Submitted by Alexander Schwinn @alexxcons

Description

Activity

Submitted by Alexander Schwinn `@alexxcons`