[Aptitude-devel] Changesets 00288a0dcae2 through 4710b5c94fcc

Obey Arthur Liu arthur at milliways.fr
Thu Dec 4 05:19:54 UTC 2008


Daniel Burrows a écrit :
>   Hi everyone,

Hi!

>   I'm back from vacation, and I've been trying to catch up on the work
> that went on while I was gone.

Hope you had a good vacation  :)

>   Right now we have several almost-redundant ways of doing the same
> thing.  The context menu and Package menu are identical to each other,
> and the big fat buttons in the description area do a subtly different
> thing (when several packages are selected, the menus act on all of them
> but the buttons only act on the one whose description is displayed).
> Leaving technical issues aside, is this too much redundancy in the UI,
> or is shoving the actions in the user's face more important?

I think that what is displayed right now is ok, as far as "shoving in
the face goes"

>   Would it perhaps be better to have a second "package" toolbar?  I
> think I decided against this because I felt like it would inevitably be
> too cryptic: the great thing about the buttons is that they say exactly
> what they do.

We don't have much screen real estate left anyway.

> changeset:   2205:00288a0dcae2
> description:
> Implemented a basic duplicate of the contextual popup menu in the menubar.
>  This is a reversal from my previous commit.
> ====
>
>   In addition to reversing the previous commit, this introduces a bunch
> of new code:
>
> ====
> +  void EntityTreeView::on_cursor_changed()
> +  {
> +    // TODO: we might plan to do some more elaborate filtering
> +    //       based on which tab is being active. If not, we may
> +    //       as well revert to a simple signal instead of this
> +    //       virtual function.
> +    signal_selection_change();
> +  }
> +
> ====
>
>   This seems like a completely unused and undocumented function, maybe
> left over from some intermediate version of the source tree.  Can it
> just be pruned?

I mentionned it somewhere else but that's because only detecting cursor
changes is not enough to reliably detect where the "interface focus" is.
It's probably a bad idea to put code about that in a function called
on_cursor_changed(). I should stick with the signal and put the relevant
code someplace else.

>   Also, the above comment is wrong: on_cursor_changed isn't a virtual
> function.

I was thinking about the two ways events can be detected and acted upon
in GTK+, on_whatever() virtual functions and on_whatever_signal signals.

> ====
>    Gtk::Menu *
>    EntityView::get_menu(const std::set<PackagesAction>&actions,
> -                      const sigc::slot1<void,
PackagesAction>&callback) co
> nst
> +                      const sigc::slot1<void, PackagesAction>&callback,
> +                      Gtk::Menu * rval) const
>    {
> -    Gtk::Menu *rval(manage(new Gtk::Menu));
> +    if (rval == 0)
> +    {
> +      rval = manage(new Gtk::Menu);
> +    }
> ====
>
>   I don't like this idiom: it adds an unnecessary complication to the
> interface of get_menu().  I've changed get_menu() to fill_menu(),
> which takes a menu as a parameter and inserts actions into it.

Right.

> ====
> +  void EntityView::package_menu_handler()
> +  {
> +    Glib::RefPtr<Gtk::TreeModel>model = get_model();
> +    Glib::RefPtr<Gtk::TreeView::Selection>selected =
tree->get_selection();
> +    if(selected)
> +      {
> +        std::set<PackagesAction>actions;
> +        std::set<pkgCache::PkgIterator>packages;
> +
> +        Gtk::TreeSelection::ListHandle_Path selected_rows =
selected->get_selected_rows();
> +        for (Gtk::TreeSelection::ListHandle_Path::iterator path =
selected_rows.begin();
> +             path != selected_rows.end(); ++path)
> +          {
> +            Gtk::TreeModel::iterator iter = model->get_iter(*path);
> +            cwidget::util::ref_ptr<Entity>ent = (*iter)[cols.EntObject];
> +            ent->add_actions(actions);
> +            ent->add_packages(packages);
> +          }
> +
> +        std::cout << "package menu handler : ";
> +        for (std::set<pkgCache::PkgIterator>::iterator it =
packages.begin();
> +        it != packages.end(); ++it)
> +        {
> +          std::cout << " " << it->Name();
> +        }
> +
> +        std::cout << std::endl;
> +
> +        if(!actions.empty())
> +          {
> +            get_menu(actions, sigc::mem_fun(this,
&EntityView::apply_action_to_selected), pMainWindow->get_menu_package());
>            }
>        }
>    }
> ====
>
>   This looks good, except that it unnecessarily builds a set of the
> packages it's acting on and then writes them to standard output.  I
> presume that this is left-over debugging code.  We could use that set
> to rebuild the menu when the package states in question change -- but I
> think we'd be better off always rebuilding the menu whenever any package
> states change (less book-keeping and should be reasonably inexpensive).
> I've committed that change (renaming the handler for more clarity at the
> same time), so the Package menu updates correctly now.

Debugging yeah. As I said before, the code is not very accurate as far
as guessing where the interface focused packages are.

>   The Package menu still doesn't apply to the currently active tab.  I
> think that in order to handle this, we'll have to allow tabs to know
> whether they're currently selected.  I'll implement a flag on each Tab,
> along with signals when a tab is selected or deselected (with a
> guarantee that deselection is sent before selection).

Useful.

>   I suspect that we should also move fill_menu() to the main window
> class or to gui.cc.  It's quite generic anyway, and we might want to
> generate the Package menu from some other source.  Working on that right
> now.

Well, some of this code should be merged with the pop-up context menu code.

>   I also noticed a lot of this:
>
> (aptitude:7476): glibmm-CRITICAL **: Glib::ObjectBase*
Glib::wrap_create_new_wrapper(GObject*): assertion `wrap_func_table !=
0' failed
>
> (aptitude:7476): glibmm-WARNING **: Failed to wrap object of type
'GtkAccelLabel'. Hint: this error is commonly caused by failing to call
a library init() function.
>
> (aptitude:7476): Gtk-CRITICAL **: gtk_bin_remove: assertion
`GTK_IS_WIDGET (child)' failed
>
>   I'm not sure where it's coming from, but it should be tracked down
fixed.

I might have rushed the code out of the door a little hastily.

I might have been unclear about the whole point of these chanagesets. As
you may have noticed with the first (later reversed) changeset, I made a
lot of ambitious changes and then just dropped the whole branch out of
frustration from arising issues. I later rethought it a started writing
the later changesets.

The point here is that I want most actions that can be done with the
mouse to be done with the keyboard. At least, enough to have a decent
keyboard-only experience that's not too far from the ncurses one. GTK+
is not very good at that but I think it's doable.
There's also this GUI design rule that says that all the actions should
also be available through the menus (ok, openoffice and office pre-2007
are bad design examples). GTK+ seems to enforce some of this by only
allowing accelerators (keyboard shortcuts) to be simply defined with
menu items. On another hand, that's a good idea because the user knows
to look for shortcuts by in the menus. It's what the ncurses interface
does (although there are also some "secret" shortcuts, in the sense that
they don't appear in the menus but only in the documentation).

The pop-up context menu code creates menus on-the-fly about the selected
packages. It's quite easy to know which packages the user is thinking
about in this case: it's those right under the mouse.
It gets more complicated with the menubar "package" menu. We don't
really know which selection the user is thinking about when he goes to
the package menu and the menu has to be created *before* he tries to
click (or finally doesn't) on the menu. That's why this menu gets
recreated all the time. It's not very pleasant but I haven't figured out
how this can be done on-the-fly. Gtk::Menu and Gtk::MenuShell don't seem
to offer ways to do this. Maybe I should have a look at how the others
do it.

I googled the seemingly standard "how do I duplicate the potential
pop-up context menu in the menubar" issue but nothing useful turned up.

Any idea ?

Cheers

Arthur

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 260 bytes
Desc: OpenPGP digital signature
Url : http://lists.alioth.debian.org/pipermail/aptitude-devel/attachments/20081204/f2c42c4e/attachment.pgp 


More information about the Aptitude-devel mailing list