Fix revealer using unmap event

[peteruithoven]

Fixes: elementary/wingpanel-indicator-bluetooth#64
Supersedes: #174

After the bluetooth indicator disappears it sometimes doesn't properly re-appear.
This can be reproduced by running:

systemctl restart bluetooth

I can also reproduce the issue by rapidly switching nightlight on off. You see it sometimes not showing up or even showing only half of the icon:


(Using Enter to switch the button makes this easier)

I noticed I could fix this by commenting the delayed re-ordering:
https://github.com/elementary/wingpanel/blob/master/src/Widgets/IndicatorEntry.vala#L82-L85
But this means the indicator isn't removed and you'll see extra margins.

Removing the previous timeout and increasing the interval helps a great deal. But that means another arbitrary value, which could still give issues on slower systems and adds a delayed sudden jump in the indicators.

I was hoping to find a solution like: #155

This PR's uses the unmap signal to know a indicator is hidden, it then start a 0ms timeout to request the menubar reordering. The timeout makes it cancelable when the indicator is quickly made visible again.

Still feels like quite a workaround, I'm open for better solutions.
Disconnecting from the unmap signal when the indicator is made visible again gave issues for some reason.
It could be that a better solution is doing a less crude reordering in the menubar?

[jeremypw]

I'll need a little time to get up to speed with the code base before reviewing ...

[jeremypw]

I can confirm that it fixes the problem - I'll have a think about whether a more elegant solution exists.

[jeremypw]

@peteruithoven : A simple solution that seems to work is to add an Idle loop in IndicatorMenubar.apply_new_order (). Then you can remove all the related Timeouts in Indicator.vala.

[peteruithoven]

I'm afraid that when I remove the timeout stuff and change the apply_new_order to the following, I still have the issue that the indicator sometimes doesn't appear of even only appears half way.

    public void apply_new_order () {
        Idle.add(() => {
            clear ();
            append_all_items ();
            return false;
        });
    }

Maybe there is a better way to make the reorder call cancelable than with a timeout?
Like I mentioned I tried disconnecting from the unmap when the indicator was made visible again, but that caused the Wingpanel to crash when enabling, disabling quickly.

[jeremypw]

@peteruithoven: OK, I guess it must depend on hardware characteristics - I could not trigger the bug using a simple Idle (but I could only occasionally trigger it in master anyway). You could try using Idle.add_full with GLib.Priority.LOW and/or also keep a reference to the source id so that you can cancel the Idle if apply_new_order is called too rapidly. It seems that the fix should be kept within IndicatorMenubar if possible even if it means using a Timeout there.

There are probably more complex solutions using async functions and cancellables and/or mutexes.

[jeremypw]

Don't forget to install the changes and restart wingpanel - I have sometimes forgotten!

[peteruithoven]

You could try using Idle.add_full with GLib.Priority.LOW and/or also keep a reference to the source id so that you can cancel the Idle if apply_new_order is called too rapidly.

I'm happy to report that that seems to work. Did I implement it like you meant it?

I kind of prevent that mistake by temporarily removing wingpanel from the monitored list of cerbere and using the following the following command to compile and run:
ninja && sudo ninja install && G_MESSAGES_DEBUG=all wingpanel

[jeremypw]

Good that it works! The only minor problem is that it will give critical warnings in the terminal if the source no longer exists when you try to remove it so you need to test whether it is greater than zero before removing it and set it to zero at the end of the Idle closure. The id should also have a default value of zero.

[jeremypw]

@peteruithoven : Could you try this?

        base_indicator.notify["visible"].connect (() => {
            if (menu_bar != null && (revealer.get_reveal_child () != base_indicator.visible)) {
                /* order will be changed so close all open popovers */
                popover_manager.close ();

                if (base_indicator.visible) {
                    popover_manager.register_indicator (this);
                } else {
                    popover_manager.unregister_indicator (this);
                }

                set_reveal (base_indicator.visible);
                menu_bar.apply_new_order ();
            } else {
                set_reveal (base_indicator.visible);
            }
        });

It works for me.

[jeremypw]

Ah! I see you have already pushed some changes before I edited my comment - sorry.

[peteruithoven]

@jeremypw thanks I forgot about that error, I've added a check.

You're last suggestion prevents the Revealer hide animation, because it's removed immediately?

[jeremypw]

Ah, OK. I'll approve as is then.