Midori Web Browser

Merge lp://qastaging/~mboevink/midori/vim-functionality-additions into lp://qastaging/midori

  1. vim-functionality-additions
  2. Merge into trunk
Proposed by Matthew Boevink
Status: Superseded
Proposed branch: lp://qastaging/~mboevink/midori/vim-functionality-additions
Merge into: lp://qastaging/midori
Diff against target: 104 lines (+33/-2)
1 file modified
midori/midori-browser.c (+33/-2)
To merge this branch: bzr merge lp://qastaging/~mboevink/midori/vim-functionality-additions
Reviewer Review Type Date Requested Status
gue5t gue5t Needs Fixing
Cris Dywan Pending
Review via email: mp+189465@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-10-02.

To post a comment you must log in.
Revision history for this message
Cris Dywan (kalikiana) wrote : Posted in a previous version of this proposal

Please avoid accidental whitespace changes, it makes the diff look bigger when reviewing when there's no code changes.

> return false
Please use FALSE always in C code.

The "gboolean lower_g = FALSE;" looks a bit fagile - I see no code resetting it. How about adding it to struct _MidoriBrowser and reset it to FALSE when switching tabs?

review: Needs Fixing
Revision history for this message
Matthew Boevink (mboevink) wrote : Posted in a previous version of this proposal

> Please avoid accidental whitespace changes, it makes the diff look bigger when
> reviewing when there's no code changes.
>
> > return false
> Please use FALSE always in C code.
>
> The "gboolean lower_g = FALSE;" looks a bit fagile - I see no code resetting
> it. How about adding it to struct _MidoriBrowser and reset it to FALSE when
> switching tabs?

No worries bud, patched in :)

Revision history for this message
gue5t gue5t (gue5t) wrote :

I would suggest that you eliminate the ad-hoc state-machine of the "lower_g" variable entirely. If we don't intend to support a full vim language, it's much simpler to approximate things like this: treat a single 'G' as the hotkey for the ScrollTop action and a single 'g' as the hotkey for the ScrollBottom action. Hard-coded hotkeys are something of a code smell and hurt customizability. Also, action names should be capitalized like "CamelCase" rather than like "CamelCAPS".

review: Needs Fixing
Revision history for this message
Matthew Boevink (mboevink) wrote :

> I would suggest that you eliminate the ad-hoc state-machine of the "lower_g"
> variable entirely. If we don't intend to support a full vim language, it's
> much simpler to approximate things like this: treat a single 'G' as the hotkey
> for the ScrollTop action and a single 'g' as the hotkey for the ScrollBottom
> action. Hard-coded hotkeys are something of a code smell and hurt
> customizability. Also, action names should be capitalized like "CamelCase"
> rather than like "CamelCAPS".

It's vim, the command is pretty standard among all editors based off it, as well as vimperator etc. Seems silly to do half a vim command.

lp://qastaging/~mboevink/midori/vim-functionality-additions updated
6429. By Matthew Boevink <email address hidden>

Fixed camelCase problem. Sorry about that

Unmerged revisions

6429. By Matthew Boevink <email address hidden>

Fixed camelCase problem. Sorry about that

6428. By Matthew Boevink <email address hidden>
6427. By Matthew Boevink <email address hidden>

Adds vim G and gg functionality

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: