Re: Putty Shapes - changes

[Nadim Shaikli <shaikli at yahoo dot com>]

--- ahmad khalifa <ahmadkhalifa at hotmail dot com> wrote:
> Simon Tatham wrote:
> >yet various issues abound possibly due to it being done
> >in the improper location, etc.
> 
> Added Shaping according to Simon's recommendations...
> 
> New Patch available with an extra file... from the CVS
>
>   http://cvs.arabeyes.org/cvsweb/projects/external/putty/
> 
> can i make a patch file that adds a file..? i mean do patches have to
> be between original and new files only, or can the patch add a file that
> doesnt have an original... i.e in this situation do i have to update 
> arabic.c and the .patch or can i append arabic.c to the .patch ...?

Yup, you sure can - you simply do a 'diff -u /dev/null ./path/my_file'
(tricky I know :-)  I'll fix up the patch unless you get to it before
me.

> will this reach Simon..?? nadim !

Not unless you CC him :-)  Do please do this next time so we don't end-up
re-mailing him things he needs to see.
 
> >OK; I do have some comments.
> >
> >First (and easiest), I think I'd be happier if large pieces of new
> >code went into a separate source file (arabic.c? shaping.c? bidi.c?
> >Not sure what it should be called...) rather than being added to the
> >already oversized terminal.c.
> 
> umm, i choose arabic.c if u dont mind, and the only additions to
> terminal.c and window.c are only 1 function call each and a few
> useless comments...
> 
> i included the arabic.c file in the MSVC project, did not do any #includes 
> btw, Nadim, i stole the header off VIM's arabic.c file, and added my name
> and a link to Arabeyes... :))  and i also changed the README file..., would
> u mind reviewing the 2 files nadim, and telling me if anything is wrong or
> missing..??

Sure and you didn't steal anything - that's how we work, we build on each
others' work ;-)  I'll look into 'em soon'ish.

> >I think that the Arabic shaping shouldn't be being called just
> >before do_text(). The redraw routine divides each line into
> >(potentially) small sequences of characters which have the same
> >attributes and can be redrawn in a single ExtTextOut call. Calling
> >do_arabic_shape() just before do_text() causes it to process each of
> >these sequences individually, whereas I'd have expected you to want
> >to process the line as a whole. Calling do_arabic_flip() inside the
> >ExtTextOut wrapper function has the same problem: it means that the
> >positions of characters will move around depending on what size the
> >redraw chunks are.
> 
> do_arabic_flip() only flips the letters in a word, so it benefits from the
> division of the line into words according to their attributes... anyway as
> soon as Bidi is implemented, do_arabic_flip() is going out the window...

Yup, once Bidi is included this will not be needed.
 
> umm, u know what its actually pretty useless now because you have to
> force a redraw on putty for it to work, so maybe i'll remove it now...
> 
> >I would have thought that the sensible place to do _both_ shaping
> >and bidi was at the top of the `for' loop in do_paint(). (That for
> >loop begins on line 3395 of the current unpatched terminal.c.) The
> >loop body begins with
> >
> >        scrpos.y = i + term->disptop;
> >        ldata = lineptr(scrpos.y);
> >
> >so that `ldata' then contains an array of _all_ the characters on
> >the line, before they are split up into small chunks for redrawing.
> >My understanding is that what you want to do is to process `ldata'
> >immediately after this, to change the order of the characters and do
> >shaping.
> 
> ok, done..
> 
> >(It's just possible that the inner for loop, starting on line 3414,
> >might need to be split into two, so that the first loop would
> >process selection highlights, blinking and the cursor position, and
> >the second loop would deal with breaking the line up into
> >subsections for redraw. Then shaping and bidi could be done in
> >between the two loops, and the cursor position would naturally
> >adapt.)
> 
> i dont see why we need to break the inner loop into 2..?
> 
> >Your thread complains that backspacing deletes 4 characters. I would
> >imagine (though I don't have a Windows machine to test on) that this
> >is because you've modified pwrite() to print high-bit-set characters
> >literally, but you haven't modified plen() to return the right
> >number of backspaces needed when deleting them again.
> >
> >(That stuff needs changing anyway for UTF-8; pressing backspace
> >during local line editing should delete one _character_, which isn't
> >necessarily one byte.)
> 
> Exactly, i changed pwrite()... what do u think of the changes..?
> and what other changes to plen() do u suggest..?
> 
> >Also I see this comment:
> >  he said in his chat session that it should be done at the top of
> >  the big FOR loop.. between that and the display function inside
> >  the loop, all FEXX values are changed to 06XX
> >I don't understand this comment. Is it a problem you're having with
> >the existing PuTTY code?
> 
> sorry, my mistake..., i used to Shape on term->disptext, according
> to your irc session log... when i did that, term->disptext would contain
> the shaped Data (Form-B FEXX) but the function do_text() was called
> with the Unshaped data (06XX)... i figured it out now... because do_text()
> is called with data from ldata not disptext....
> nevermind that...
> 
> anyway, here are the remaining issues...
> 
> + deletes 4 Arabic letters, (plen() needs modification)
> + i think maybe Windows manages to sneak in... i'll check that...
>   because some letters are flipped, maybe an error in my flipping function
>   (i dont think anyone will understand what im talking about... :-)
> + LAA is not shaped.. (ligatures)
> + need to recheck the shaping code again...
> + how do we turn Arabic On and OFF, or do we just leave it always on..?

I think we'll need an ON/OFF button (somewhere ?) with it defaulting
to ON this would include the Bidi as well of course.

> + do u want to add ant #defines and #ifdefs simon..???
> + typing LAA with the Keyboards own LAA key, generates two letters
>   which are not arabic... !!! (i dont remember that)...
> + Bidi...
> + ask Simon if its OK doing the shaping on the unsigned long array...?
>   or do we need to extract the letters and do the 2 loop thing...??!!
> + some spacing is too much...
> 
> My Schedule: (i've seen posts about Scheduling, am i doing it right..?)
> in the next 7 to 10 days i'll recheck the Shaping code, and see about 
> adding Bidi, if the shaping is stable, and efficient and accepted by
> Simon, i'll add something to PuTTy's Blurb :)))

Great, alot of what we can/can't do will hinder on Simon giving us
direction and setting us straight, we'll also need resolution on
which Bidi code (license issues) we can use as we have yet to hear
back from Owen and Simon on ICU.

> btw, Tripod kicked me, because i was using the account as storage, so
> i wont be able to provide an .exe, any1 willing to Host it for Testers..?

There are many other options out there (get a yahoo account and use
its free geocities.com/USERNAME) or you can check the various free
hosts listed in,

  http://www.free-webhosts.com

worse yet, I can do it on your behalf :-)

> btw2, Anyone willing to test PuTTy..?

Yup, I VERY much am and I'm sure other bums on this list would do it too.

> OK, Your move Simon...

Simon, do please let us know (after looking at the patch in CVS) what
we should do and if the changes sit well with you.  As always if we
need to touch base in a faster manner, there is always IRC.

Regards (& great work Ahmad).

 - Nadim


__________________________________
Do you Yahoo!?
Get better spam protection with Yahoo! Mail.
http://antispam.yahoo.com/tools