[DynamicNotebook] The name of "tab_removed" signal is misleading

Bug #1181657 reported by Julián Unrrein
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Granite
Fix Released
High
Niclas Lockner

Bug Description

From the current trunk (http://bazaar.launchpad.net/~elementary-pantheon/granite/granite/view/head:/lib/Widgets/DynamicNotebook.vala) :

public void remove_tab (Tab tab) {
            if (Signal.has_handler_pending (this, Signal.lookup ("tab-removed", typeof (DynamicNotebook)), 0, true)) {
                var sure = tab_removed (tab);
                if (!sure)
                    return;
            }

            var pos = get_tab_position (tab);
            if (pos != -1)
                notebook.remove_page (pos);
            tab.page_container.destroy ();
        }

"tab_removed" is emitted before deleting the tab in cuestion, with the apparent purpose of letting a developer interfere with the event of a tab removal, up tthiso the point of cancelling the tab removal altogether. So, in the end, maybe a tab wasn't removed at all.
In light of its purpose, a more sensible name for the signal would be something along the lines of "tab_removal_requested"

Related branches

Revision history for this message
Julián Unrrein (junrrein) wrote :

A concrete example:
I was experimenting with using DynamicNotebook in pantheon-files.
I wanted to bake the tab bar disappear whenever there were only one tab.
I connected an anonym function to the tab_removed signal, which would hide the tab bar when the number of tabs reached one.
When there are two tabs, and one of them is closed, the anonym function reports that there are still two tabs (because the signal is fired up before actually closing the tab) and so, it doesn't work.

Changed in granite:
status: New → Confirmed
Cody Garver (codygarver)
Changed in granite:
importance: Undecided → High
milestone: none → 0.3-beta1
tags: added: dynamic-notebook pantheon-files
Revision history for this message
Victor Martinez (victored) wrote :

Julián,

If you have some time, please submit a patch and we'll review it ASAP. We are very interested in getting DynamicNotebook into Pantheon Files. Otherwise, I'll submit a patch as soon as I can.

Thank you in advance!

Revision history for this message
Julián Unrrein (junrrein) wrote :

Victor,

I just did a quick experiment, replacing the old notebook with the new one, and modifying what was necessary to make it work, but it looked promising.
Unfortunately I don't have much free time now to finish it. But I could upload what I have done so far, if you'd like to take a look at what I did.
It wasn't difficult - DynamicNotebook takes care about a bunch of stuff that is currently done on Files' side. Like keyboard shortcuts handling and mouse scroll over the tab bar.

I could see 3 issues remaining:
-When using the "new tab" event invoked from the tab bar (new tab button or keyboard shortcut <Ctrl+T>, for example) Files has a high chance of force closing.
My guess (from console output, I didn't debug it) is that Files tries to grab properties from the new tab (to store them or something) as they were present before. This doesn't happen when inserting a new tab explicitelly.
-Setting the tab title. The widget inside a tab (ViewContainer) contains a Gtk.Label that changes when needed. That widget itself was used in the old tab style, so it updated itself automatically. As we can only access to a string property to change a Granite's Tab title, the previous arrangement isn't viable.
If ViewContainer was implemented in Vala, then a quick solution would be to fire a signal each time the label is modified. If it isn't, I don't really know if there is an easy way to work around this issue.
-When only one tab is open, closing it should open a new default tab (i.e. don't allow the tab bar to be empty). This is where this bug comes to play.

Regards

Revision history for this message
Julián Unrrein (junrrein) wrote :

About this bug, I think that the purpose of the signal is to allow the programmer to interfere in the event that a tab removal is requested. It even allows to cancel the removal (when a function connected to the signal returns false).

So the issue would be that the signal's name is misleading, since tab removal may not happen at all.

summary: - DynamicNotebook emits tab_removed signal before actually deleting a tab
+ [DynamicNotebook] The name of "tab_removed" signal is misleading
description: updated
Revision history for this message
Casper Christiansen (fault) wrote :

Perhaps "tab_removal_requested" would fit the name better? Accompanied with a post deletion "tab_removed" signal being called if the request wasn't aborted.

This change would break a lot of applications though. How are breaking API changes usually handled? I'm guessing by continuing support for the old way but calling it deprecated? I guess the post deletion signal could be called "on_tab_removed" and calling both "tab_removal_requested" and "tab_removed" to check for cancellation.

Sorry I'm still rather new to software development.

I've attached a patch with the implementation I had in mind. I would like some feedback on whether my approach is acceptable first, before I create a merge proposal.

Changed in granite:
assignee: nobody → Casper Christiansen (fault)
status: Confirmed → In Progress
Changed in granite:
assignee: Casper Christiansen (fault) → nobody
status: In Progress → Confirmed
Niclas Lockner (niclasl)
Changed in granite:
assignee: nobody → Niclas Lockner (niclasl)
Niclas Lockner (niclasl)
Changed in granite:
status: Confirmed → In Progress
David Gomes (davidgomes)
Changed in granite:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.