[Vimperator] [patch] Mappings refactoring

[Виктор]

В пт, 2007-05-25 в 13:10 +0200, Martin Stubenschrott написа:
> On Fri, May 25, 2007 at 12:33:16PM +0300, Виктор Кожухаров wrote:
> > Hi
> 
> Hi, nice you found some time to do work on vimperator again :)
> 
> > I've done some work on the mappings. The lowdown is that I moved them a
> > new object, under V.prototype.mappings.main (since there should be
> > V.prototype.mappings.user for user defined mappings) as a multiarray
> > with the first dimension being the mode.
> 
> I am not convinced by that as I want to objectize mappings (like
> commands).
> 
> So my idea was to have two classes like:
> 
> function Mapping(cmd, action, extra_info) // extra info is a hash
> {
> 	//cmd and action are required
> 	this.command = cmd;
> 	this.action = action;
> 	// all others are optional
> 	if (extra_info)
> 	{
> 		if (extra_info.usage)
> 			this.usage = extra_info.usage;
> 		if (extra_info.help)
> 			this.help = extra_info.help;
> 			...
> 	}
> 	else
> 	{
> 		// do some heuristics where possible for non-specified args
> 	}
> }
> 
> 
> function Mappings()
> {
> 	// private
> 	var mappings;
> 	var user_mappings;
> 
> 	// public
> 	// would be called like:
> 	// vimperator.mappings.add(new Mapping("/", vimperator.search.openDialog,
> 	//            { usage: "moo",
> 	//              help: "find it"}
> 	this.add(mapping);
> 
> 	// note that mapping is not a object of the mapping class but a
> 	// string, and command is any string
> 	// because a user should be able to map:
> 	// :map x :open foo.com<Return>
> 	// then mapping="x" and command=":open foo.com<Return>"
> 	// this should internally create a Mapping object, like this:
> 	// { user_mappings.push(new Mapping(mapping, function() {vimperator.commands.normal(command)}); // v.c.normal will be the ex command :normal
> 	this.addUserMapping(mapping, command);
> 
> 	// later maybe:
> 	this.removeUserMapping(mapping); // only for user mappings, main
> 	                                 // mappings never should be removed
> 
> 	// TODO: how to handle incomplete mappings (add a also_incomplete
> 	// param and return an array of partial commands if set?)
> 	// also this.get() should be an intelligent function and aware of
> 	// main and user mappings and return a Mapping() object which is
> 	// then ready to be called
> 	this.get(mapping_str);
> }
>From what I understand, Mapping is a function to add users mappings? But
why not just have a method in the Mappings object that does that, like :
add: function(mode, cmd, action, extra_stuff), and remove: ... And with
the api you propose, where do the default maps get stored? My
proposition doesn't look that different. I still add a mappings object,
but I haven't implemented the add/remove/get methods yet. I think that
add/remove should only work on user mappings, since we have to have a
way to return to the default mappings. I think vim itself never
overwrites the default mappings, but looks in the user mappings first.
> 
> Well this may be an incomplete api, but that's how I want it.
> I am sorry, you did the work, and I can't commit it in this stage, but
> for bigger things like that, it's probably better to discuss the design
> on the ML before doing the implementation.
> But I don't have time until monday anyway to do much coding, so if you
> have time and want to hack around, the repo shouldn't change that much.
> 
> > I also added a new maptype
> > field to the normal mappings, so that we could eventually support
> > mappings with parameters (I need this to implement the marks mappings)
> > or motion mappings.
> 
> Ah great, i thought about how to add pending commands like needed for
> macros or marks anyway, didn't find a good solution. but this is, great.
> 
> Put please make the modes a public hash like:
>     this.modes = { // actually not private, but Firefox complains if this doesn't come first
>         // main modes
>         NONE:             0,
>         NORMAL:           1 << 0,
>         INSERT:           1 << 1,
> 	}
> ... in vimperator.js instead of global consts (which I want to get rid off)
> 
> > also while i was doing that, i noticed that the new search is mapped
> > under 'g/'. why is that? I added the '/' as a map to it, since I didn't
> > even know we had a new search until i looked at it. so why is the 'g/'
> > even necessary?
> 
> Because the search is buggy, and can't be enabled by default until the
> only show-stopping bug is fixed:
> 
> Go to www.orf.at (or I think any other site with frames), and search
> with g/, you will see, it doesn't work and even breaks the find for
> other tabs. Interessting catch:
> If you serach with / (the default find of firefox) first on this page,
> then you can search ONCE with g/ and then you have to use / again before
> you can use g/ again.
> 
> I and waawaamilk from IRC haven't found the problem yet, therefore it
> stays disabled for now.
I've been having a quick look inside the find.js file, and so far I
think that's not the correct approach. Most of the code can be made
redundant by just reusing useful function from gFindBar. Reusing
gFindBar.highlightDoc would obsolete a lot of the code, since that
function does the searching and highlight everything. We can also reuse
the finding code itself, or just take it and add regex support to it.
> 
> There are some other minor issues, but this one bug has to be ironed
> out, before I will change g/ to /, see the wiki for other thigns:
> http://vimperator.cutup.org/index.php?title=Search
> 
ok, the wiki is news to me.
> PS: CC'ing to the ML, because the mapping design could be of interest to
> others as well.
-- 
Виктор Кожухаров /Viktor Kojouharov/