On Fri, Oct 19, 2001, Michael Nordstr�m wrote:
> On Fri, Oct 19, 2001, MATSUMOTO Masakazu wrote:
> > Anyway i want your opinion.
> 
> I might have time to take a closer look at the code on Sunday,

I've been busy working on the Linux version of the viewer, but
I have looked a little closer at your changes.

- I don't know if it is such a good idea to run MainFormInit without
  first erasing the main form -- could result in a memory leak.

- ScribeMemo should probably not be included in externalform.c.
  I think all the functions involved in the copying of the text
  should be added in a file of their own.

- The change that is intended to stop the rendering of a paragraph
  after the last visible line has been displayed stop too soon.

  You can check for yourself by scrolling down pixel by pixel in
  a paragraph of text. The last "line" will be empty until you have
  scrolled the height of the line at which time it will "magically"
  appear.

- The code should check for LargeImage before any copy attempt is
  made.

- The max extentY value will never change for a device (except for
  the Handera, but it will still not change while the paragraph is
  rendered), so it is unnecessary to store that value in a variable.

- The assignment of inView in DrawParagraph should be moved to 
  WriteLine. It is only used in that function and the value it 
  depends on will not change when WriteLine is executed. Will make
  it possible to remove the global variable.

  BTW, why add yet another variable that should be checked in the
  if-cases when there already is a variable used to keep track of
  whether the text should be rendered or not? Replace inView with
  tContext->writeMode (remember to restore its value at the end of
  WriteLine).

- The writing of the URL should be moved to the ANCHOR_END case in
  HandleFunctions. Makes more sense to put it there and it also 
  makes the test much simpler (it will be enough to check if the
  hardcopy mode is active)

- Nitpicking: In OpenMemo you have a variable called null (='\0')
  and then in ScribeMemo you have a similar variable called eol
  (this time a string "\0", but only the first byte will be used,
  so why not use a char?). Anyway, they should use the same name
  (I would suggest eol, since null should be NULL and nothing else).

- Adding a newline in the memo after each paragraph will make it
  look much better (IMHO).

/Mike

Reply via email to