...

It is complaining about 'r' being declared twice below. This looks more like a reassignment than a redeclaration to me. How do we properly fix it?. If all we care about here is 'r' having function scope, can't I

just get rid of the 'Range* ' typecast following the else { ? Or do I have to change 'r' to 'not_r' following the else?

Getting rid of the second 'Range* ' was the trick. Verify and then push patch :)

I'm aware that these patches have been labeled as C++ "semantics" changes. Yet we are not providing any usability test plans. Verifying a patch allows building is only half the recovery.

Although I am learning, much of these C++ patching discussions remain over my head. No different than speaking in a foreign language. :)

Somebody should propose a usability test plan to ensure the functional intent of the patched code still works.

If indeed a patch is purely semantics, then I would like to see a list of project members who have been deemed qualified to render those decisions. Then I can push patches with ease-of-mind.

If not purely semantic, a usability test might be nothing more than verifying a specific window appears, a mouse-click still functions, etc.

My point is a regularly raised topic in Trinity discussions is increased bugginess. A basic quality control plan is needed with our patching efforts. That does not mean every patch needs a 10-page test plan. Many don't.

Yes, I need to be pointed to articles that a C++ noob like me can read that explains these semantic differences. I suspect most of these semantic differences are learned by experience and seldom introduced upon in formal teaching.

Darrell

On 04/18/2012 02:12 PM, E. Liddell wrote:

On Wed, 18 Apr 2012 13:25:43 -0500 "David C. Rankin" drankinatty@suddenlinkmail.com wrote:

...

On 04/17/2012 06:22 PM, David C. Rankin wrote:

...

Darrell, all

I'm making an attempt at fixing k3b for gcc47. If anyone else has already done this please stop me. It is part of bug 958.

This is another redeclaration issue, but it is not the same type of simple iterator issue I've seen earlier. The build of k3b fails on gcc47 with:

k3baudioeditorwidget.cpp: In member function 'virtual void K3bAudioEditorWidget::mousePressEvent(TQMouseEvent*)': k3baudioeditorwidget.cpp:674:12: error: redeclaration of 'K3bAudioEditorWidget::Range* r' k3baudioeditorwidget.cpp:668:14: error: 'K3bAudioEditorWidget::Range* r' previously declared here

It is complaining about 'r' being declared twice below. This looks more like a reassignment than a redeclaration to me. How do we properly fix it?. If all we care about here is 'r' having function scope, can't I just get rid of the 'Range* ' typecast following the else { ? Or do I have to change 'r' to 'not_r' following the else?

If I'm understanding the code correctly, I would probably move the declaration of r outside the condition for the if, that is:

void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) { m_draggedRange = 0; m_draggedMarker = 0; bool end; Range* r = findRangeEdge(e->pos(), &end);

if (r) { m_draggedRange = r; m_draggingRangeEnd = end; } else { r = findRange( e->pos() ); d->movedRange = r; d->lastMovePosition = posToMsf( e->pos().x() ); m_draggedMarker = findMarker( e->pos() ); } setSelectedRange( r ); TQFrame::mousePressEvent(e); }

To my eye, that's clearer than the original. I would never declare a variable inside a condition for a branching construct, even if it's legal to do so (the technique has valid applications in loop constructs, but here it just serves to muddy the scoping). </opinionated>

Alternatively, basic scoping rules suggest:

void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) { m_draggedRange = 0; m_draggedMarker = 0;

bool end; if( Range* r = findRangeEdge( e->pos(), &end ) ) { m_draggedRange = r; m_draggingRangeEnd = end; setSelectedRange( r ); } else { Range* r_two = findRange( e->pos() ); d->movedRange = r_two; d->lastMovePosition = posToMsf( e->pos().x() ); setSelectedRange( r_two ); m_draggedMarker = findMarker( e->pos() ); }

TQFrame::mousePressEvent(e); }

Darrell, E (don't know first name)

I agree with all your comment and both proposals above, but like Darrell, I don't have a c++ background to know the 'best' way to do this. I certainly agree what declaration within the conditional looks bad, but I was not going to take the liberty and re-write it given the conservative approach I take when monkeying with the code (though I like the declaration outside the if statement much better as suggested). I know they are not big changes, but I don't know enough to know if they were declared in the if for some other reason.

Of the two, I like the first suggestion the best, just from a style/clarity standpoint:

void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) { m_draggedRange = 0; m_draggedMarker = 0; bool end; Range* r = findRangeEdge(e->pos(), &end);

if (r) { m_draggedRange = r; m_draggingRangeEnd = end; } else { r = findRange( e->pos() ); d->movedRange = r; d->lastMovePosition = posToMsf( e->pos().x() ); m_draggedMarker = findMarker( e->pos() ); } setSelectedRange( r ); TQFrame::mousePressEvent(e); }

I'll change it any way you guys want it changed. The way it is currently patched works fine (k3b runs fine as well), but it isn't the prettiest. Let me know which option you like the best and I'll redo the patch. I'm just glad k3b is building and working on gcc47 without and -fpermissive kludge :)

On 04/18/2012 02:12 PM, E. Liddell wrote:

void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) { m_draggedRange = 0; m_draggedMarker = 0; bool end; Range* r = findRangeEdge(e->pos(),&end);

if (r) {
  m_draggedRange = r;
  m_draggingRangeEnd = end;
}
else {
  r = findRange( e->pos() );
  d->movedRange = r;
  d->lastMovePosition = posToMsf( e->pos().x() );
  m_draggedMarker = findMarker( e->pos() );
}
setSelectedRange( r );
TQFrame::mousePressEvent(e);

}

What say we do it like this:

void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) { Range* r = findRangeEdge( e->pos(), &end ); m_draggedRange = 0; m_draggedMarker = 0; bool end;

if (r) { m_draggedRange = r; m_draggingRangeEnd = end; setSelectedRange(r); } else { = findRange( e->pos() ); d->movedRange = r; d->lastMovePosition = posToMsf( e->pos().x() ); setSelectedRange( r ); m_draggedMarker = findMarker( e->pos() ); }

TQFrame::mousePressEvent(e); }

Here is why -- looking at the setSelectedRange code it has something to do with repainting the screen, which brings up the question in my mind: "Does setSelectedRange( r ); have to come 'before' m_draggedMarker = findMarker( e->pos() ); -- as it is originally. That's the only reason I could think of that the original author would have put it both after the 'if' and after the 'else' -- otherwise, I agree with you, just put it at the end.

I'm not sure how to check in the code to see if need to be before the findMarker( e->pos() ); call. Can somebody who knows this stuff let us know?

Either way, the proposed patch above preserves the order of the calls -- the only remaining question is can we git rid of both setSelectedRange(r); calls by moving a single call below the conditional... Small potatoes there... Let me know. Thanks.

On 04/18/2012 03:36 PM, David C. Rankin wrote:

On 04/18/2012 03:26 PM, David C. Rankin wrote:

...

else { = findRange( e->pos() );

with the 'r' = of course :)

Here is what I have come up with from the additional posts here. I've updated the patch to bug report 958. The link to the new patch is here:

http://bugs.pearsoncomputing.net/attachment.cgi?id=549

The code reads:

void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) { Range* r = findRangeEdge( e->pos(), &end ); m_draggedRange = 0; m_draggedMarker = 0; bool end;

if (r) { m_draggedRange = r; m_draggingRangeEnd = end; setSelectedRange( r ); } else { r = findRange( e->pos() ); d->movedRange = r; d->lastMovePosition = posToMsf( e->pos().x() ); setSelectedRange( r ); m_draggedMarker = findMarker( e->pos() ); }

TQFrame::mousePressEvent(e); }

You guys that do the approval before Darrell submits, take a look and let us know if it is good. I'm on to the next one..

On 18 Apr 2012, E. Liddell stated:

On Wed, 18 Apr 2012 15:52:26 -0500 "David C. Rankin" drankinatty@suddenlinkmail.com wrote:

...

Range* r = findRangeEdge( e->pos(), &end ); m_draggedRange = 0; m_draggedMarker = 0; bool end;

[...]

Are you sure that compiles? I would have assumed end needed to be declared before using a reference to it as an argument to findRangeEdge(), which was why I put things in the order that I did. Or maybe I'm wrong.

Nope, you're right, that's the rule.