John,
On Mon, 2002-10-21 at 05:52, John Levon wrote:
> On Mon, Oct 21, 2002 at 05:42:10AM +0930, Darren Freeman wrote:
>
> > No I mean that as I turn the wheel, the scroll rate changes as I go
> > past, say, lots of headings or bigger writing.
>
> Hold on to your hat: lyx cannot easily scroll by non-row based amounts.
Holding...
> This is because we use setCursorByCoordinates() which works on a
> row-level not pixel level. Since Headings are tall rows, you end up
> scrolling "faster".
I guessed something like that =( <letting go of hat, relief showing>
> This sucks and has sucked for a long long time.
I agree. It's really noticeable with a wheel because the page speed is a
bit weird.
BTW you should get yourself a wheel mouse if you can.. They're really
groovy. But if you do have to go for an MS one I advise getting a
solvent and removing the MS from it =) Unless you want your warranty in
which case you lose twice =(
> > I don't know why so many files are mentioned at the start of the diff,
> > but the files I actually changed are listed correctly.
>
> Just ignore that.
>
> > So now how do I refresh my view of the tree after you've sorted through
> > it?
>
> What do you mean ?
Well, if you like them, then you will add them. Am I right? Well then I
would like to refresh whatever you have done to the tree.
> Here's a quick review of your changes :
>
> int BufferView::scroll(long time)
> {
> - return pimpl_->scroll(time);
> + return pimpl_->scroll(time); // note that time is now called offsetLines - DF
> }
>
> Please put comments /above/ the source line not to the right.
Noted.. Bad habit, comes from wanting people to notice the comment
because it needs fixing.
> Also please change the paramter name here as well :)
> And in the header if appropriate.
I stayed out of BufferView because I didn't know why there was also a
BufferView_pimpl (maybe the latter is a teenaged version?)
I'll let you do that =) I'll break something, just you watch me..
> -int BufferView::Pimpl::scroll(long time)
> +//
> +// Wheel mouse scroll, move by offsetLines multiples of text->defaultHeight().
> +// Observing hard-coded boundaries to prevent scrolling off the document.
>
> This comment should go in the header BufferView_pimpl.h, something like
> this :
It does, it's a duplicate. I think all such comments should be
duplicated like I have done. But it's up to you.
> /**
> * Wheel mouse scroll, move by offsetLines multiples of
> * defaultHeight(), limited by the top/bottom of the document.
> */
> int scroll(long offsetLines);
>
> +int BufferView::Pimpl::scroll(signed long offsetLines)
>
> Why add "signed" ? It is implicit.
Because it signals intent to the reader. How many bugs have you seen
that were related to somebody forgetting that their type was implicitly
signed or unsigned, while coding at 5am?
> + if(!buffer_)
> + {
> return 0;
> + }
>
> LyX style dictates :
grrr... oh well... you're the boss, Boss.
Clearly we need a WYSIWYM code editor.. Can LyX edit it's own code yet?
=)
> if (!buffer_) {
> return 0;
> }
>
> + LyXText const *t = bv_->text;
>
> LyXText const * t ...
>
> + int lineHeight = t->defaultHeight();
>
> int const ...
>
> + //
> + // Scroll by the desired displacement, observing the document limits.
> + //
>
> Why two empty comment lines ?
My style, which I consider to be more readable with them than without
them. I admit it took a day to get used to, but I've never looked back.
Also my style dictates that two empty lines precede one of those comment
blocks, and one after. This attaches it to the code which follows.
> + signed int newFirstY = t->first_y + disp;
>
> Not that you really displacing by pixels not "lines" here. I think - so
> you should change the comments/names to match this fact.
Oh well.. It worked =) I would be tempted to find another value for
lineHeight to avoid the changing scroll speed. If there were a global
way of finding the defaultHeight() for the standard environment then it
would be what I was after. Currently it changes depending on the style
you're looking at at the time.
> - scrollDocView(int(diff));
> + if(newFirstY < minFirstY)
> + {
> + newFirstY = minFirstY;
> + }
>
> again
> if (newFirstY < minFirstY) {
> newFirstY..
>
> and you could use std::min/std::max
More info please. You mean if (newFirstY < std::min) etc? OK. Didn't
know they were there =)
> + //
> + // Update the document scrollbar.
> + //
> +
> workarea().setScrollbarParams(t->height, t->first_y, t->defaultHeight());
>
> This comment is pointless. What additional information does it give ?
Well to a person who just opened a text editor on this file, having
never seen LyX source, it gives an enormous amount of information.
And as a habit I document every logical section of code with a similar
heading. It improves readability enormously, and reduces the time you
spend looking at statements as you remember what they do. I know that
the statement I documented was obvious to you, you've been staring at
the code for a while I gather... But after 6 months of absence are you
sure the comment wouldn't be handy?
> +
> + //
> + // Done.
> + //
> +
> return 0;
>
> Same here.
>
> }
>
> + /**
> + * Wheel mouse scroll, move by offsetLines multiples of text->defaultHeight().
> + * Observing hard-coded boundaries to prevent scrolling off the document.
> + */
>
> Oh OK. Then you don't need the duplicated description I mentioned above.
>
> I hope you don't think me too pedantic - it is quite important that new
> lyx code match the agreed-upon coding style as much as possible. This
> inevitably means compromises (there are some things I don't agree with
> in the style too, but I do them nonetheless).
I agree about consistency. But then I also agree that that file was a
shocker to read when you've just checked out the code for the first time
=)
That was an example of how I would document the code if I went rampant
through the tree. It would slowly all end up like that. In a perfect
world ;)
> thanks
> john
Have fun,
Darren
