Re: [Fwd: Re: set of text scrolling patches (pageUp:/pageDown:) (+ Bonus ProjectCenter Patch :) )]

[Nicola Pero]

>   Hi Nicola,
> 
> after I applied your patch to GSSimpleLayoutManager, I lost one of my 
> patches when recreating the cummulative patch.
> 
> Here is revised version 2 :-)

Thanks David, I applied most of it - definitely the pageUp/pageDown, and
the fix for the font panel key equivalent.  Thanks!  Very good stuff. :-)



+   * Take verticalPageScroll into accout, but try to make sure
+   * that amount is never negativ (ie do not scroll backwards.)
+   * FIXME: Maybe we sould send messages instead of accessing
+   * ivars for verticakPageScroll as subclasses may override it,
+   * but for now be consistent with the rest of the code.

If you want to change the verticalPageScroll, you call
setVerticalPageScroll:.  There is no need to subclass it. :-)

If a subclass wants to change the vertical scroll, it can call
setVerticalPageScroll: as well (in -initWithFrame: I suppose).

So I don't see any advantage in sending a message to itself to access its
own ivar in this case ... in practice it would just be a useless slow-down
- the API already gives you all the flexibility you need.

There might definitely be cases in which sending a message to itself to
access an ivar might be a very good idea for more powerful subclassing etc
- but in this case in practical terms it's just a waste of resources
(which, I think, is why we access all those variables directly).



+/**
+ * Tries to move the selection/insertion point up one line in the
+ * receiver while trying to maintain the horizontal position of the
+ * last vertical movement.
+ * If the reciever is a field editor, this method returns immediatly. 
+ */
 - (void) moveUp: (id)sender
 {
+  float    cachedInsertPointX;
+  unsigned oldGlyphIDX;
+  unsigned newGlyphIDX;
+  unsigned newCharIDX;
+  NSRange  oldGlyphRange;
+  NSRect   oldGlyphBounds;
+  NSPoint  iPoint;
 
   if (_tf.is_field_editor)
     return;

+  if ([_textStorage length] < 0)
+    return;
+  
+  /*
+   * Take the textContainerInset into account
+   */
+  iPoint.x = _originalInsertPoint        - _textContainerInset.width;
+  iPoint.y = NSMidY(_insertionPointRect) - _textContainerInset.height;
+
+  oldGlyphIDX = [_layoutManager glyphIndexForPoint: iPoint
+                                 inTextContainer: _textContainer];
+
+  oldGlyphRange = NSMakeRange(oldGlyphIDX,0);
+  oldGlyphBounds = [_layoutManager boundingRectForGlyphRange: oldGlyphRange
+                                  inTextContainer:_textContainer];
+
+  /* 
+   * Determine the position we aim to move the cursor to
+   * on the next line 
+   */
+  iPoint.y = NSMinY(oldGlyphBounds) - 1;

This is precisely where I was not really convinced.  It's sort of a
philosophical question :-) ... why 1 ?  why not 2 or 0.3 or 1000 ?

I know it's because in practical terms, 1 works well with the normal fonts
under the normal scaling (you probably thought you'd never get a font
smaller than 1 in points/pixels).

But let's say that we have a wonderful text backend which can scale fonts
arbitrarily well, and you rescale the coordinate system in your textview
so that a font of 10 points had a height of 0.5 in that coordinate system
(NSView allows you to rescale arbitrarily the coordinate system if you so
wish).

Am I correct that your code would then move up/down by 2 lines at a time
instead of 1 ?

This is the 'philosophical' reason why at the moment the code to compute
where you go when you go up one line is in the layout manager ... because
the layout manager knows about lines, and no matter how big (or small)
they are, it can compute properly, while the textview would need to move
up/down by a fixed tiny arbitrary amount which might be incorrect in
extreme cases.

Of course, we might decide we don't care :-) I'd like to hear your opinion