Closed Bug 338374 Opened 18 years ago Closed 18 years ago

Mac doesn't support non-solid line drawing


(Core Graveyard :: GFX: Mac, defect)

1.8 Branch
Not set


(Not tracked)



(Reporter: brettw, Assigned: jaas)



(Keywords: fixed1.8.1)


(7 files, 6 obsolete files)

The Mac SetLineStyle code is here:

It just saves the line style away. This variable seems totally unused except for the getter/setter, and the getter is never called by the Mac graphics code.

For reference, the GTK version is here:

You might be able to use PenPat:

This affects inline spellchecking: the misspelling underlines are solid and look bad.
Flags: blocking1.8.1?
OS: Mac OS X 10.3 → Mac OS X 10.2
Hardware: PC → Macintosh
Attached patch fix v1.0 (obsolete) — Splinter Review
Attachment #222518 - Flags: review?(mark)
Talking to Mark this morning, he said that we should do this call right before drawing the line (in DrawLine) and reset it immediately afterwards because the PenPat affects more than just lines.
Attached patch fix v2.0 (alternative) (obsolete) — Splinter Review
This is alternative patch. Instead of doing anything with QD when the line style is set, it sets QD to the appropriate pen pattern before drawing lines and sets QD back to the 'black' pen pattern after drawing. This way the pen pattern can only affect the line drawing operation.
Patch #2 doesn't seem quite right--to match the Windows & Linux (branch) rendering, the "dotted" style should be one pixel on, one pixel off, but it looks like the results are two pixels gray, two pixels on.  (This is on an Intel iMac running 10.4.6.)
Comment on attachment 222518 [details] [diff] [review]
fix v1.0

v2 is better
Attachment #222518 - Flags: review?(mark) → review-
Comment on attachment 222523 [details] [diff] [review]
fix v2.0 (alternative)

I'd prefer it if you called GetPenState to get the current penpat and restore it when you're done drawing your lines.
(In reply to comment #4)
> Patch #2 doesn't seem quite right--to match the Windows & Linux (branch)
> rendering, the "dotted" style should be one pixel on, one pixel off, but it
> looks like the results are two pixels gray, two pixels on.

The gray is the background color, you can change it with ::RGBBackColor.
We really want the non-dashes to be transparent. It that possible?
Attached patch fix v2.1 (obsolete) — Splinter Review
This patch makes dotted lines have 1-pixel dots. It also does save/restore of RGB back color and pen state.

I don't know of a way to draw dotted lines transparently short of completely rewriting the way we draw lines, so I just made the back color on dotted/dashed lines white all of the time. I'm well aware of the fact that such an approach sucks, and I'm happy to hear ideas.
Attached image Screenshot of patch 2.1
Looks more correct now--is there no way to sample the background color to make it "transparent"?  I'm concerned about textboxes with odd background colors.
What you're saying about sampling background colors is what I mean by rewriting the way we draw lines. That is probably possible but a pain in the butt.

We may be able to do something with SetPortPenPixPat, set up a 1-bit mask for the graphics port and get transparency that way. With 1-pixel dotted lines the potential for problems here is that our fill color pixels will overlap with the holes in the mask.

Another option which might be somewhat better is to sample the color at the start point of the line and set that to the background color for the whole line. The problem then is that if the line passes through an area with different background colors we're in trouble. Or if the start point for the line is not the right color, say if the start is on the hanging part of a letter in a white-background text box.
I tried sampling the color at the start point of the line and another big problem is what happens if we paint twice. The first time it works fine - we sample a white pixel and that becomes the spacer color. Problem is, the second time we sample a red pixel because we just drew the line there and then we draw a solid red line because the spacer is red. It is easy to see this in the spellchecking example - if you misspell a word it initially paints the line correctly, but when you type the next word it repaints the line to solid red. Not that I had high hopes for this approach anyway...

A somewhat more successful version of this hack is sampling the pixel to the right of the origin of the line. This pixel is always an off-pixel so we don't have the double-draw sampling problem. However, this won't work as well for vertical lines. We could try to determine if a line is mostly vertical or mostly horizontal and sample up or to the right based on the results. We could sample a few pixels off for the dashed line. If anyone is interested in trying this, I can post a patch.
Attached patch "smart" sampling fix, v3.0 (obsolete) — Splinter Review
This does "smart sampling" as outlined in the second part of comment 12, also including logic for taking line direction into account. It works well for the spellchecking case, and should work well in most cases that might actually happen.

Note: The smart sampling logic is only implemented for dotted lines, not dashed lines (spellchecking uses dotted). I could implement it for dashed lines if people like this approach.
Mark suggested using the transparent mode for CopyBits, and now that some more about QuickDraw is clear to me I understand how that might work. I'll write a patch for that approach soon.
Attached patch fix v4.0 (obsolete) — Splinter Review
It looks like this is going to work with CopyBits. This way we can have actual transparent pixels between the dots in the line. This patch works great for the spellchecking example with a red line over a white textbox.

What worries me about this patch is what happens when the line is black or white, or the background it is over is black. Those cases I have not tried yet, everything else should be fine.

Brett - can you point me to the code where I can change the color of the spellchecking underline line? If this patch checks out in those cases I'll request review.
Attachment #222518 - Attachment is obsolete: true
Attachment #222523 - Attachment is obsolete: true
Attachment #222565 - Attachment is obsolete: true
Attachment #222576 - Attachment is obsolete: true
The patch on bug 338209 changes the wavy underline code. The code on trunk is here:
On branch there is similar code.
Attached file test html
This is a good test case for my patches.
Attached patch fix v4.2Splinter Review
This patch should do everything correctly for regular lines, dotted lines, and dashed lines, including transparency. It works in all cases for me, different background colors and different lines colors.

I was messing around with optimizing this a bit by not making the offscreen gworld as big as the one it is copying into, but I decided against it. I doubt it is much of a penalty, certainly not as much as we use it. Another issue with that is when you're trying to make a gworld to fit just a 1-pixel-tall horizontal line (for example), the gworld rejects the bounding rect for that. So if your line goes from (10,22) -> (100,22), the gworld will reject the bounding rect for that line and just become really really big. You'd have to make the gworld at least one pixel bigger on all sides than whatever you are bounding, and that just makes this probably unnecessary optimization messier. The code in this patch is pretty clean and easy to understand.
Attachment #222681 - Attachment is obsolete: true
Attachment #222999 - Flags: review?(mark)
Comment on attachment 222999 [details] [diff] [review]
fix v4.2

Looks good - all comments were given/accounted for on irc, just add a comment at the top of each block saying that the code is duplicated and why it would have been heinous to move it off into its own proc.  It would have been nice to do smaller copies, but oh well.
Attachment #222999 - Flags: review?(mark) → review+
Attachment #222999 - Flags: superreview?(brettw)
Comment on attachment 222999 [details] [diff] [review]
fix v4.2

I don't think I'm allowed to give SR for this module. Looks good to me, though.
Attachment #222999 - Flags: superreview?(brettw) → superreview?(mikepinkerton)
(In reply to comment #20)
> Created an attachment (id=223074) [edit]
> Screenshot of 4.2--looks good!

The question is, does the suggest feature offer "base" as an alternative for "sfo?"
Comment on attachment 222999 [details] [diff] [review]
fix v4.2

Attachment #222999 - Flags: superreview?(mikepinkerton) → superreview+
checked in on trunk
Target Milestone: --- → mozilla1.8.1alpha3
This is important for spellchecking to draw correctly so targetting 1.8.1a3
Definitely too late for a3. B1 should be fine, though.
Attached patch fix v5.0 (obsolete) — Splinter Review
I didn't know about the transparent pen mode and how that interacts with patterns until Mark mentioned it might work today. Turns out it does. This should fix the Tp problems.
Attachment #223381 - Flags: review?(mark)
Comment on attachment 223381 [details] [diff] [review]
fix v5.0

r=me, but I want you to make two changes to both methods:

Call ::RGBBackColor with the result of ::InvertColor of the foreground color, and restore ::RGBBackColor to the saved initial state obtained from ::GetBackColor.


Add this check as the first part of each method:

  if (mLineStyle == nsLineStyle_kNone)
    return NS_OK;

Those are straightforward enough that you can make them on checkin and I'll look at it later if you don't feel like posting a new patch now.
Attachment #223381 - Flags: review?(mark) → review+
Attached patch fix v5.1Splinter Review
implement mark's suggestions
Attachment #223381 - Attachment is obsolete: true
Comment on attachment 223389 [details] [diff] [review]
fix v5.1

I wanted you to invert the fore color, not the back color.
Attachment #223389 - Flags: review-
my bad, fixed on trunk
Trunk fix looks good.
Attachment #223452 - Flags: review?(mark)
Attachment #223452 - Flags: approval-branch-1.8.1+
Comment on attachment 223452 [details] [diff] [review]
1.8.1 branch fix v5.2

Attachment #223452 - Flags: review?(mark) → review+
moving on out, will get this for beta1
Target Milestone: mozilla1.8.1alpha3 → mozilla1.8.1beta1
landed on 1.8.1 branch
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Flags: blocking1.8.1?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.