Re: [patch] middle click panning and related

[Krzysztof Foltman <kfoltman portal onet pl>]

Lars Clausen wrote:

> I was about to apply this, but some of the details in it are still too
> messy.  For the panning, I still don't like having a new global variable
> for the previous tool.  It should instead be passed to create_scroll_tool
> and stored in the ScrollTool structure.

I disagree. The ability to use a tool in a transient way is (IMHO) a 
property of selection mechanism and not a tool, and shouldn't be 
implemented in each tool separately. "Select tool as transient" vs 
"Scroll tool is transient" (it's not, it's only used that way).

If you (say) add a Rotate or Autoconnect or Reparent tool and want to 
use it as a transient tool, you'll have to copy the transient selection 
code from ScrollTool. Not good I think. What more, imagine how restoring 
previous tool would work. Either the selection code would have to peek 
into ScrollTool (breaks encapsulation => ugly), or a "deactivate" tool 
function in ScrollTool would have to select the previous tool (I wanted 
to avoid that too, for the same reason).

The previous tool might be stored in the Tool structure as well, but is 
that property really separate for each tool ?

In a scripting-capable C++ program, I'd probably have a class Toolbox 
with methods like: (pseudocode)

enum {
  SELECT_TOOL_ONETIME=0, // force non-sticky
  SELECT_TOOL_STICKY=1, // force sticky
  SELECT_TOOL_UIDEFAULT=2, // default UI setting (sticky or non-sticky)
  SELECT_TOOL_UIREVERSE=3, // reverse of default UI setting
  SELECT_TOOL_TRANSIENT=4, // transient mode: remember previous choice 
(if not transient), restore
};
vector<Tool *> enum_tools(); // optional
Tool *get_default_tool(); // the sticky tool
Tool *get_current_tool(); // won't return a transient tool!
int get_current_tool_flags();
Tool *get_transient_tool(); // non-NULL if we're in transient mode
Tool *get_tool_by_id(ToolId id);
string serialize_tool(Tool *); // if we want to preserve current tool 
during script execution or something
Tool *unserialize_tool(string);
void select_tool(Tool *, int flags=SELECT_TOOL_UIDEFAULT);
int tool_flags_from_modifier_keys(int modifier_keys); // returns 
SELECT_TOOL_UIDEFAULT or _UIREVERSE for now

...or something like that.

Comments ? (except memory management problems with dynamically created 
tools).

>  For the tool stickiness reversing
> -- which UI-wise is nice -- adding an extra parameter to a bunch of central
> calls just for that is ugly -- better to pass in at least the full set of
> modifiers so they could be used for other purposes.

Set of modifiers... ok, but what if the central call is not used from 
UI, but (say) a scripting language or external API ? I think central 
calls should not depend too much on the UI itself. Of course, it could 
make sense to make the new parameter a set of flags (but not 
tightly-tied to modifier keys). Then, a mapping function may be added 
for mapping a modifier set to flag set (with right choice of flag 
values, it might be a simple &).

Imagine we have an API call that takes a modifier key set and then 
someone decides that Dia 1.50 will have different modifier keys. Or that 
someone adds an API-only flag (not used as a modifier key bit in Gtk+), 
then a new version of Gtk+ is released, where this flag is set on every 
second mouse click. Instant disaster.

Krzysztof