vimprobable-users

[Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!

I used the chance to test the new mailinglist to public a refactured hinting.js. I spent some time in cleaning up the hinting code and added some changes so that the hinting acts like in the vimperator.

Changes i made:
- Ported hinting javascript in to object to make the code cleaner.
- Used speaking varibale names.
- Also removed useless code and added some utility methods
- Adde early breakout to hinting.js
- Also reduced some if else nesting by logical reduction
- Added focusHint() function and used cloning to generate hintelements
- Made some methods private
- Reduced number of variables in hinting javascript. 
  Hints are now saved with some metadata (element, number, text, span,
  backgrouncolor, color) into one array
- Removed extravariable for focused element in hinting.js
- Implemented removing of hints if a number is selected. That means that hint 
  are removed that don't match a given number like in vimperator. I you type
  1 in hintingmode all hint except of [1,10-19] will be removed. The   
  hintnumber will not be updated and it's already possible to tab or shift-
  tab through the remaining hints.
- Implemented possibility to fire focused hint by return key. I hope this   
  feature will make other hintingfeteatures easier to implement

Sorry for the squash patch, was much easier for me to generate.

I this we should made some decision where to handle the different to 
implement hinting actions (open link, copy linktext to clipboard, focus hinted element). In my opinion this could be given via parameter to the
js-function that generates the hints, so we can later if the hint is selected simply use fire() to handle the action. This should also keep the
c-code cleaner.

I could not find the path for copy the hinted url to clipboard that was mentioned in a previous mail, i think i should it merke into the patch.

Daniel

-- 
Daniel Carl
Gustav-Fischer-Straße 33
07745 Jena
0176/20166455




NEU: FreePhone - 0ct/min Handyspartarif mit Geld-zurück-Garantie!		
Jetzt informieren: http://www.gmx.net/de/go/freephone

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hannes S. <ha...@yl...>]

Hello Daniel,

without getting into the coding issues right now (that should be the
second step), let's talk about the behaviour/policy changes this patch
introduces first.

"Daniel Carl" <dan...@gm...> wrote:
> - Implemented removing of hints if a number is selected. That means
> that hint are removed that don't match a given number like in
> vimperator. I you type 1 in hintingmode all hint except of [1,10-19]
> will be removed. The hintnumber will not be updated and it's already
> possible to tab or shift- tab through the remaining hints.

I see some advantages of this behaviour, but also some potential
disadvantages. You did the right thing insofar that the hint numbers
don't change. It give the users better overview or confuse them due to
visual irritation (I'm undecided). It could be a problem that not all
hintable elements are clearly visible anymore if users want to correct
their selection. I'd like to hear more opinions.

> - Implemented possibility to fire focused hint by return key. I hope
> this feature will make other hintingfeteatures easier to implement

I'm not sure what the change is here. Return already fires the active
hint, doesn't it?

> I this we should made some decision where to handle the different to 
> implement hinting actions (open link, copy linktext to clipboard,
> focus hinted element). In my opinion this could be given via
> parameter to the js-function that generates the hints, so we can
> later if the hint is selected simply use fire() to handle the action.
> This should also keep the c-code cleaner.

I disagree. The way I envision it (and the way I implemented the other
hinting modes), the Javascript code is the universal part and the
different actions/targets are remembered and implemented in C. Already,
the idea is to simply use fire() - that passes the selected data back
to the C layer, nothing else. The C layer needs to know what action the
hints have been requested for anyway (since this is where the request
originates). I don't see any advantage in passing that information
which we already have at that level to an even higher one in order to
implement more logic there. But maybe I'm misunderstanding your point.

> I could not find the path for copy the hinted url to clipboard that
> was mentioned in a previous mail, i think i should it merke into the
> patch.

http://www.vimprobable.org/mailman/archives/vimprobable-users/2011-June/000797.html

Hannes

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Andraž 'r. L. <ru...@co...>]

:2011-08-10T15:32:Hannes Schüller:

> I see some advantages of this behaviour, but also some potential
> disadvantages. You did the right thing insofar that the hint numbers
> don't change. It give the users better overview or confuse them due to
> visual irritation (I'm undecided). It could be a problem that not all
> hintable elements are clearly visible anymore if users want to correct
> their selection. I'd like to hear more opinions.

Hmm I'm split on this as well. It might reduce the clutter but it might
be annoying.

-- 
Andraž 'ruskie' Levstik
Source Mage GNU/Linux Games/Xorg grimoire guru
Re-Alpine Coordinator http://sourceforge.net/projects/re-alpine/
Geek/Hacker/Tinker

I may not agree with what you say, but I'll defend your right to say it.

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi Hannes,

thank your for your answer to the patch.

> I see some advantages of this behaviour, but also some potential
> disadvantages. You did the right thing insofar that the hint numbers
> don't change. It give the users better overview or confuse them due to
> visual irritation (I'm undecided). It could be a problem that not all
> hintable elements are clearly visible anymore if users want to correct
> their selection. I'd like to hear more opinions.

If the user changes his select the hints are shown again, like the hinting
works on current upstream revision. 

1) If the user hits a number all not matching hints will be removed. If the
   user deletes last number all possible hints will appear like expected. 
1) The user hits a number and after that he decides to filter by text, the
   previous hints will also be removed and only the text matching ones will be
   shown also those hints that where removed by the numbered hinting.

But your are right, if it's irritation it should be removed from the patch. I
think I where in flow an tried to adapt the behavior of vimperator.

> > - Implemented possibility to fire focused hint by return key. I hope
> > this feature will make other hintingfeteatures easier to implement
> 
> I'm not sure what the change is here. Return already fires the active
> hint, doesn't it?

For numbered hint filtering it's true. But if you filter by text, there is
none hint focused or active and you can't activate it by return.

> I disagree. The way I envision it (and the way I implemented the other
> hinting modes), the Javascript code is the universal part and the
> different actions/targets are remembered and implemented in C. Already,
> the idea is to simply use fire() - that passes the selected data back
> to the C layer, nothing else. The C layer needs to know what action the
> hints have been requested for anyway (since this is where the request
> originates). I don't see any advantage in passing that information
> which we already have at that level to an even higher one in order to
> implement more logic there. But maybe I'm misunderstanding your point.

For now it's OK so, but what is with the mentioned link actions prefixed with
';'? For example yank hint URL to clipboard? Either a dedicated function in
JavaScript is called or the fire function should know what to return, or did I
misunderstood the code?

To soothe you, I'm disimpassioned about the way where the different hint
actions are handled, but I agree with you that we should decide us for one
straight way and don't mismatch the concerns between the C layer and the
JavaScript.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!

At work I'm confronted with some cruel webapplications that uses frames and
a lot of hidden or overlayed elements. I'm dependent to use these application
for my daily work. Therefor I started to refacture the hinting.js to deal with
hinting also on framebased pages.
It's only a first attempt to make vimbrobable to work a little better on
framesites. Also if the hinting within frames could not fire the hints into
the right target, it's makes my work easier. I believe the hinting within
frames will become a more important thing if new hinting actions will be
implemented.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hannes S. <ha...@yl...>]

Hi!

Daniel Carl <dan...@gm...> wrote:
> Therefor I started to
> refacture the hinting.js to deal with hinting also on framebased
> pages. 

This sounds marvelous and I have to dig into this subject in detail
this weekend! For now, just one question as I'm slowly losing overview:
Are these patches replacements of your previous patch, are they on top
of it or something completely unrelated?

Hannes

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi Hannes,

I know that the patches are really cruel, have to many changes at the time,
so it's not remarkable that you loose the overview. The patches should be
applied on top of the previous one.

To get the history of the stashed patch, you can have a look at the branch
on github https://github.com/fanglingsu/vimprobable/commits/fls/hinting-refacture.

Daniel
-- 
NEU: FreePhone - 0ct/min Handyspartarif mit Geld-zurück-Garantie!		
Jetzt informieren: http://www.gmx.net/de/go/freephone

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hannes S. <ha...@yl...>]

Hi,

first review:

- The patches you posted don't apply cleanly on top of each other. This
  is due to whitespace breakage.

- Talking of whitespace breakage, the first patch seems to be
  especially messy in this regard. Some parts of the code are hardly
  readable after applying it.

- From a functional point of view, I like this very much! As you
  already said, we will probably have to find a way to preserve the
  target information (I see your point about relocating this logic
  into Javascript now). The way it is now is already very useful,
  though.

- Using the patch, I get the following message on the console: "@1:
  Unsafe JavaScript attempt to access frame with URL
  http://... from frame with URL http://... Domains, protocols and
  ports must match."

Hannes

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hannes S. <ha...@yl...>]

Hi,

after applying all patches in succession, hints don't appear in this
version at all anymore. Also, there is now a tremendous amount of
history in these patch series. I think it might be time to create a
new, clean patch series to begin systematic issue tracking?

Hannes

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hans-Peter D. <hpd...@gm...>]

Hi

On 17:03 Sat 20 Aug     , Daniel Carl wrote:
> Hi!
> 
> I hope the hinting is now functional like before refacturing.
> Fixed bug that caused vimprobable to keep in hintmode, even if hint was fired.

Sounds cool, but I'm too lazy to figure out what patches to apply onto
what and Hannes seemed to have problems with that as well.

Your patches appear to be generated with git, so it would be really cool
if you could just upload a git branch somewhere (in addition to posting
the code to the list, of course). Then people could just clone/push to
try your changes which would certainly increase your testers/user base ;)

Thanks,
HP

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!

On Sat, Aug 20, 2011 at 08:49:46PM +0200, Hans-Peter Deifel wrote:
> Your patches appear to be generated with git, so it would be really cool
> if you could just upload a git branch somewhere (in addition to posting
> the code to the list, of course). Then people could just clone/push to
> try your changes which would certainly increase your testers/user base ;)
> 
> Thanks,
> HP

I have and upstrem git repository at
https://github.com/fanglingsu/vimprobable.git the branch with the changes is
the "fls/hinting-refacture".

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!
On Sat, Aug 20, 2011 at 08:03:47PM +0200, Hannes Schüller wrote:
> after applying all patches in succession, hints don't appear in this
> version at all anymore. Also, there is now a tremendous amount of
> history in these patch series. I think it might be time to create a
> new, clean patch series to begin systematic issue tracking?

I think I've startet refacturing on the wrong branch. Maybe this was the
reason, that the patches didn't apply on top of eachother. I've rebased the
branch to top of vimprobable2 and made all patches again. Hope they will work.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!

I have found out a missbehavior of hinting on some pages that uses JavaScripts
that overwrites the Array constructor. On this pages for(e in foo) doesn't
work as expected. This patch replaces this kind of for loops by more
traditional way of iterating over the arrays.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hannes S. <ha...@yl...>]

Daniel Carl <dan...@gm...> wrote:
> On Wed, Aug 24, 2011 at 11:13:02PM +0200, Hannes Schüller wrote:
> > I'd say the problem is a difference in the Webkit versions. 100000
> > still works for me, 1000000 doesn't. So I'd say we use something
> > like 100000 for reasons of compatibility.
> Thank you for inspecting the issue. Can you make the patch with al
> lower value?
> I'm wondering that 10000000 doesn't work.
> In http://www.puidokas.com/max-z-index/ mentioned restriction are
> higher than our current value. But on most serious pages also a
> z-index of 100000 should fit our needs.

Thanks! I'd say it's just a bug in Webkit, but you're right - the lower
value should be no issue for any practical purposes. Back to testing
now.

Hannes

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi Hannes!

> - The patches you posted don't apply cleanly on top of each other. This
>   is due to whitespace breakage.
Oh sorry, I think I have meessed up something with my local branches. Could
you merge it into your local branch? Should I try to make the pathces again to
be applied on current HEAD?

> - Using the patch, I get the following message on the console: "@1:
>   Unsafe JavaScript attempt to access frame with URL
>   http://... from frame with URL http://... Domains, protocols and
>   ports must match."
I'm not shure how to deal with it. It's a violation of the same origin policy,
that takes affect here. At the moment I found no solution to avoid the
messages, maybe a try catch at the right position will prevent it to appear.
In the current version we wouldn't be able to access frames that aren't in the
same origin policy like the page into which we insert the hinting.js.
The pentadactyl plugin for firefox can handle also frame from other domains,
so I promise we could do it also.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hannes S. <ha...@yl...>]

Daniel Carl <dan...@gm...> wrote:
> > - The patches you posted don't apply cleanly on top of each other.
> > This is due to whitespace breakage.  
>
> Oh sorry, I think I have meessed up something with my local branches.
> Could you merge it into your local branch? Should I try to make the
> pathces again to be applied on current HEAD?

I could apply them with some minor manual work, don't worry :)

Hannes

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

On Sun, Aug 14, 2011 at 11:23:04AM +0200, Hannes Schüller wrote:
> - Using the patch, I get the following message on the console: "@1:
>   Unsafe JavaScript attempt to access frame with URL
>   http://... from frame with URL http://... Domains, protocols and
>   ports must match."
I added alitte check to prevent these messages. This is only a quick fix until
we find a soution to access the also to make it hintable.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!

This patch doesn't resolve the problem and should not be applied. I disables
hinting in all frames.

Is there anybody how knows how to test if a frame has same protocol, host and
port, without accessing this properties on the frame, whate raises the error?

> ---
>  hinting.js |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hinting.js b/hinting.js
> index 2b981e8..c106654 100644
> --- a/hinting.js
> +++ b/hinting.js
> @@ -22,6 +22,9 @@ function Hints() {
>          hints = [];
>  
>          function helper (win, offsetX, offsetY) {
> +            if (topwin.location !== win.location) {
> +                return;
> +            }
>              var doc = win.document;
>  
>              var win_height = win.height;
> -- 

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi Hannes.

> - Using the patch, I get the following message on the console: "@1:
>   Unsafe JavaScript attempt to access frame with URL
>   http://... from frame with URL http://... Domains, protocols and
>   ports must match."
The loggin of these message seems to be a bug in webkit. The violation of same
origin policy sould throw an exception instead of logging only a message on
console and retriev undefined values.
https://lists.webkit.org/pipermail/webkit-dev/2010-August/thread.html#13880
http://code.google.com/p/chromium/issues/detail?id=17325

I think we have to live with this behaviour for now.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hannes S. <ha...@yl...>]

Hi!

Daniel Carl <dan...@gm...> wrote:
> On Sat, Aug 20, 2011 at 08:03:47PM +0200, Hannes Schüller wrote:
> > after applying all patches in succession, hints don't appear in this
> > version at all anymore. Also, there is now a tremendous amount of
> > history in these patch series. I think it might be time to create a
> > new, clean patch series to begin systematic issue tracking?
> 
> I think I've startet refacturing on the wrong branch. Maybe this was
> the reason, that the patches didn't apply on top of eachother. I've
> rebased the branch to top of vimprobable2 and made all patches again.
> Hope they will work.

Thanks, this seems to work! I'm giving it a stress test right now, but
the first glance looks very good. I've still got an offset of six lines
in main.c when applying the patches (starting with patch 1) though. Are
you sure you don't have any other changes in your branch? This didn't
prevent me from applying the patches, but it might hint at some change
being missed in the series.

First impressions:
- Since lowercase "f" now also respects a link's target attribute,
  target="_blank" is handled as well. This conforms to the HTML
  specification, I guess, but I somehow liked to be able to ignore this
  nonsense of opening links in a new window. Opinions?
- This logic relocation clashes with this URI handler patch:
  http://www.vimprobable.org/mailman/archives/vimprobable-users/2011-June/000781.html .
  Not necessarily a showstopper, needs to be kept in mind, though.

Hannes

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi Hannes!

On Tue, Aug 23, 2011 at 11:01:36AM +0200, Hannes Schüller wrote:
> I've still got an offset of six lines
> in main.c when applying the patches (starting with patch 1) though. Are
> you sure you don't have any other changes in your branch? This didn't
> prevent me from applying the patches, but it might hint at some change
> being missed in the series.
I'm not sure. I got one warning about a whitespace clash during I made
the patchfiles. I don't want to post again such a large number of
patches. The branch on github should work - I've spent some time to
rollback the github repositorie's master branch to the vimprobable2
upstream HEAD and rebased all branches on it. If needed I can post it
here, but I promise it's nought there.

> - Since lowercase "f" now also respects a link's target attribute,
>   target="_blank" is handled as well. This conforms to the HTML
>   specification, I guess, but I somehow liked to be able to ignore this
>   nonsense of opening links in a new window. Opinions?
I agree that 'f' should force to load the link into the current window.
For links that have the target _blank set as real attribute we can
remove it before firering it - it would be easy to implement. But there
are a number of pages that simulate the setting of the target attribute
via an eventobserver or a JavaScript function to set the target on thy
fly. I disbelieve that we would be able to open these links into the
current window.

> - This logic relocation clashes with this URI handler patch:
>   http://www.vimprobable.org/mailman/archives/vimprobable-users/2011-June/000781.html .
>   Not necessarily a showstopper, needs to be kept in mind, though.
Thank you for the hint, I'll have a look at the patch and try to adapt
the hinting to work together with it.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!
On Tue, Aug 23, 2011 at 11:01:36AM +0200, Hannes Schüller wrote:
> - Since lowercase "f" now also respects a link's target attribute,
>   target="_blank" is handled as well. This conforms to the HTML
Following patch removes target="_blank" from links that should be opened into
the current window.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!

I added a config object to the hinting.js to configure the behavior and look
of the hints and to clean the code from style settings.

If someone wants hints like pentadactyl generates could use following config
in the hinting.js.

...
hintCss: "z-index:10000000;font-family:monospace;font-size:9px;"
       + "font-weight:bold;color:black;background-color:white;"
       + "padding:0px 1px;position:absolute;opacity:0.8;border:1px solid #444;",
...

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Daniel C. <dan...@gm...>]

Hi!

The hinting added hints for ankors without href attributes. This
behavior is removed in attached patch.

Daniel

Re: [Vimprobable-users] [PATCH] Refactured hinting

[Hannes S. <ha...@yl...>]

Hi,

I've been having one minor issue: On some sites, the hints appear
behind the hinted element (making them unusable). This includes sites
which worked before, like for example this one:
http://www.goodolddays.net/forum/forum.php?id=4 (check the "New Topic"
and "New Poll" buttons).

Hannes