View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0002202JEDI VCL00 JVCL Componentspublic2004-10-08 17:142004-10-26 11:23
ReporteralmicoAssigned Touser72 
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
PlatformWin32OSWindows XPOS VersionSP2
Product Version3.00 BETA 2 
Target VersionFixed in Version3.00 RC 1 
Summary0002202: TJvXPBar causes an Access Violation when the mouse is over the last item
DescriptionI installed the latest from CVS and selected Developer Install. In file JVXPBAR.PAS, line 1732 causes the AV. The problem is due to NEWINDEX being equal to VisibleItems.Count. I saw that if you place your mouse in the upper half of the visual item, NEWINDEX is properly evaluated. Putting the mouse on the lower half of the visual item causes NEWINDEX to be beyond the limit.
The problem does not arise on the first item because there is a check for NEWINDEX<>-1. The upper part of the first items computes NEWINDEX=-1.
I guess the problem lies in the following lines of code:

Header := FC_HEADER_MARGIN + HeaderHeight + FC_ITEM_MARGIN;
if (Y < Header) or (Y > Height - FC_ITEM_MARGIN) then
  NewIndex := -1
else
  NewIndex := (Y - Header) div ((Height - Header) div FVisibleItems.Count);
TagsNo tags attached.

Activities

almico

2004-10-08 17:22

reporter   ~0005349

Doing some additional testing it looks like the wrong number is the expected item height. This would explain why I can't reproduce the same error in another TJvXPBar where there is only one item and in another where there are only 4.

almico

2004-10-08 17:28

reporter   ~0005350

In order to reproduce, the easiest way should be:
1) add 16 items to TvXPBar
2) set HOTTRACK=false
3) set SHOWLINKCURSOR=false
4) set SHOWITEMFRAME=true

then slowly start moving the mouse from the top to the bottom and look at WHEN the next item is highlighted. The use of item hints might come handy too. By the way: hint drawing interferes with item frame drawing garbling the area.

almico

2004-10-08 17:50

reporter   ~0005351

Analyzing:

procedure TJvXPCustomWinXPBar.ResizeToMaxHeight;
var
  NewHeight: Integer;
begin
{ TODO: Check this!!! }
  if IsLocked then
    Exit;
  NewHeight := FC_HEADER_MARGIN + HeaderHeight + FVisibleItems.Count * FRollOffset + FC_ITEM_MARGIN + 1;
  { full collapsing }
  if ((FRolling and not FCollapsed) or (not FRolling and FCollapsed) or
    (FVisibleItems.Count = 0)) then
    Dec(NewHeight, FC_ITEM_MARGIN);
// if Height <> NewHeight then
  Height := NewHeight - 5 + FTopSpace;
end;


I got that the correct line in HOOKMOUSEMOVE should be

NewIndex := (Y - Header) div ((Height - Header - 1 + 5 - FTopSpace) div FVisibleItems.Count);

The only problem left here is that it looks like the wrong starting offset is selected. This means that it is assigned to the HEADER part too large an offset, but I can't be completely sure about this.

Nanotonne

2004-10-12 16:08

reporter   ~0005371

for me it work! i can't reproduce your bug with your example (16 items) on delphi6/win2000pro, can you send here a simple project example for testing? (zip file).
Are your sure to download the last 2 zip from jvcl.sourceforge.net/daily/ and from jcl.sourceforge.net/daily/ ? retry and respond me here if this bug is alwas present.
what you using ? win2000pro? delphi7? thank !

user72

2004-10-13 00:57

  ~0005372

Same here. Unable to reproduce. Could you attach a test case (minimal app, source only, zipped) that exhibits the problem?

almico

2004-10-13 02:00

reporter   ~0005374

I installed the latest from CVS using TortoiseCVS.
I'm using Delphi 7 PRO and Windows XP SP2.
I investigated the source code and found that the HEIGHT of the control could be derived from the computation inside TJvXPCustomWinXPBar.ResizeToMaxHeight. If you compare that computation with what I think should be its inverse, you should agree with me that the right formula in HOOKMOUSEMOVE should be:

NewIndex := (Y - Header) div ((Height - Header - 1 + 5 - FTopSpace) div FVisibleItems.Count);

Basically, one interesting check might be an ASSERT testing whether the computed ITEMHEIGHT inside NewIndex evaluation leads to an INTEGER value or a fractionary one.

If you have a pas file that is newer than the one from CVS, feel free to add it here and I will test it.

almico

2004-10-17 03:57

reporter   ~0005412

I'm trying to connect to the CVS, but, during the past days, I was unable to. Today I investigated the DNS record for cvs.jvcl.sourceforge.net and found that it resolves to 66.35.250.209, but there is no answer from that IP. It looks like another DNS server has got a different record: 66.35.250.207.
Basically the CNAME changed from cvs.sourceforge.net to projects.sourceforge.net. Is this something you can fix or do I have to use some kind of dirty trick?
Thank you :-)

2004-10-17 04:53

 

brokenxpbar.zip (7,011 bytes)

almico

2004-10-17 04:55

reporter   ~0005415

Ok: I just uploaded a demo project that shows the issue. Simply move the mouse over the button and, when you go near the bottom of the las button in the middle bar, you should get the AV.
If not, I can post the EXE.

user72

2004-10-22 13:48

  ~0005457

>I'm trying to connect to the CVS
SourceForge changed this quite some time ago. You need to change all references from cvs.jvcl.sourceforge.net to cvs.sourceforge.net

user72

2004-10-22 13:56

  ~0005458

Try changing:
  if NewIndex <> -1 then
  begin
    if FStoredHint = '|' then
      FStoredHint := Hint;
   ...
to:
  if (NewIndex >= 0) and (NewIndex < VisibleItems.Count) then
  begin
    if FStoredHint = '|' then
      FStoredHint := Hint;
   ...

almico

2004-10-22 14:05

reporter   ~0005461

Your suggestion would address the effect, but not the cause. The problem is that the computation uses the wrong values. If you assign a hint value to each item and slowly move the mouse from top to bottom you will see that the routine improperly "guesses" the start and the end of each item. In my third message here I addressed the exact cause and proposed a fix. Why do you think my suggestion was wrong? If you apply my suggestion everything works fine.

Nanotonne

2004-10-24 07:40

reporter   ~0005473

Last edited: 2004-10-25 04:52

 with your attached project(only), i have good systematically a AV as you say/describe ... :-) you a right i think, the problem/bug was reproducted for me.

My config (on 2 PCs):
PIII 733MHz(EPOX compaq MBRD) and P4 2.4GHz (MSI MBRD)
delphi6pro on both
windows2000pro on both
screen1280x1024 on both

modifié le : 10-25-04 04:52

user72

2004-10-25 02:28

  ~0005475

> If you assign a hint value to each item and slowly
> move the mouse from top to bottom you will see
> that the routine improperly "guesses" the start
> and the end of each item.
No, it doesn't. Not for me. Regardless of whether I move the mouse fast or slow, I see the correct hint.

> Why do you think my suggestion was wrong?
Don't assume that I think your suggestion was wrong just because I proposed another fix. Since I can't see that I am getting the wrong hint value with my fix, I don't see why I should assume your fix is "better"?

As you state yourself "The only problem left here is that it looks like the wrong starting offset is selected. This means that it is assigned to the HEADER part too large an offset, but I can't be completely sure about this. "

To me this indicates that your are not 100% sure your fix doesn't have unwanted side-effects. My fix doesn't change a thing - it justs avoids using an invalid NewIndex.

I am not passing judgement here: I don't care which fix is "better" as long as it works. If you can provide confirmation that your proposed fix doesn't cause something else to fail, I have no problems using it.

almico

2004-10-25 03:21

reporter   ~0005480

First of all, thank you for your answers :-)

Your "fix", as I said earlier, avoids the effect, but doesn't fix the cause. My fix is a fix towards the cause. This makes my fix a better candidate than yours (IMHO).

As written in the third message here, "TJvXPCustomWinXPBar.ResizeToMaxHeight" procedure uses an expression to compute NEWHEIGHT. In "HookMouseMove" there is the "inverse" of that an expression. My fix is simply to use the proper "inverse" expression.

The fact that you do not get the AV might mean that your UI uses different values for borders, captions and the likes and that the "wrong" expression used in HookMouseMove works fine for you, but only because you're using the "lucky" configuration.

NANOTONNE can experience the same issue as me. Using the source and not a precompiled exe.

The fact that I seem not to be 100% sure is not because it is not the right fix (it absolutely IS), but because I saw that source code and I don't know if there are any other side effects somewhere else (due to having applied twice, or more, the wrong expression). What I actually did is writing the right code where it was wrong.

The fact that your code "doesn't change a thing" is exactly my major concern. As I already stated, the AV is the biggest problem here, but it's not the only one. Users that do have UIs configured differently from yours experience offsets in boundaries recognition.

As far as I could see, there is no problem in using my fix, but I thought that the final word was up to somebody who knew the code much better than me :-)

Thank you once again.

P.S.: who will actually post the update to the CVS?

user72

2004-10-25 11:27

  ~0005484

> Using the source and not a precompiled exe.
What precompiled exe?

> The fact that you do not get the AV might
> mean that your UI uses different values for borders,
> captions and the likes and that the "wrong" expression
> used in HookMouseMove works fine for you, but only
> because you're using the "lucky" configuration.
I am using your attached demo, so presumably I am using the same configuration as you

>As I already stated, the AV is the biggest problem here,
>but it's not the only one. Users that do have UIs
>configured differently from yours experience offsets
>in boundaries recognition.
If that is the case, you should report that as well (in a separate report)

Anyway, I've added your fix to CVS but I've also retained the check for an invalid NewIndex just in case.

user72

2004-10-26 11:23

  ~0005507

Closing this since I assume it is now resolved.

Issue History

Date Modified Username Field Change
2004-10-08 17:14 almico New Issue
2004-10-08 17:22 almico Note Added: 0005349
2004-10-08 17:28 almico Note Added: 0005350
2004-10-08 17:50 almico Note Added: 0005351
2004-10-12 16:08 Nanotonne Note Added: 0005371
2004-10-13 00:57 user72 Note Added: 0005372
2004-10-13 00:58 user72 Status new => feedback
2004-10-13 02:00 almico Note Added: 0005374
2004-10-17 03:57 almico Note Added: 0005412
2004-10-17 04:53 almico File Added: brokenxpbar.zip
2004-10-17 04:55 almico Note Added: 0005415
2004-10-22 13:48 user72 Note Added: 0005457
2004-10-22 13:56 user72 Note Added: 0005458
2004-10-22 14:05 almico Note Added: 0005461
2004-10-24 07:40 Nanotonne Note Added: 0005473
2004-10-25 02:28 user72 Note Added: 0005475
2004-10-25 03:21 almico Note Added: 0005480
2004-10-25 04:52 Nanotonne Note Edited: 0005473
2004-10-25 11:27 user72 Note Added: 0005484
2004-10-26 11:23 user72 Status feedback => resolved
2004-10-26 11:23 user72 Resolution open => fixed
2004-10-26 11:23 user72 Assigned To => user72
2004-10-26 11:23 user72 Note Added: 0005507