Bug 89382 - vcl: unwind 'TODO' comment ...
Summary: vcl: unwind 'TODO' comment ...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2015-02-14 21:04 UTC by Michael Meeks
Modified: 2016-10-25 19:11 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2015-02-14 21:04:02 UTC
These guys:

void Window::ImplAddDel( ImplDelData* pDel ) // TODO: make "const" when incompatibility ok

Look really silly - there have been no compatibility gaurentees on vcl/ for a long time. We should work out the original intent, see if it makes sense (perhaps constify the method), commit & remove the comment =)

git grep 'when incompatibility ok' -- vcl

Thanks !
Comment 1 ababaaa 2015-03-09 17:51:22 UTC
Hello ,I want to work on this issue But I need some more clarification.
Thanks for your help.
Comment 2 Soumyadip Ghosh 2015-03-20 13:39:56 UTC
Could I get some details on this please ? i want to take up this issue
Comment 3 Michael Meeks 2015-03-20 13:55:03 UTC
Um; quick reality check - go read about what the C++ "const" keyword means; absent that there is little point asking questions here. For code pointers - cut/paste the 'git grep' command I suggested.
Comment 4 Robinson Tryon (qubit) 2015-12-10 11:40:50 UTC Comment hidden (obsolete)
Comment 5 Dipankar Niranjan 2016-01-03 10:47:49 UTC
I have been pondering over this bug for about half a day now.
I guess I could do something if I understand what 'when incompatibility ok' means.
Comment 6 Dipankar Niranjan 2016-01-03 11:50:26 UTC
(In reply to Dipankar Niranjan from comment #5)
> I have been pondering over this bug for about half a day now.
> I guess I could do something if I understand what 'when incompatibility ok'
> means.
Is doing this, heading in the right direction?
http://pastebin.com/q8PxXVKL
Comment 7 Michael Meeks 2016-01-04 10:19:13 UTC
Ah - so, "when incompatibility is ok" - originally at Sun it was not possible to break internal API much at all; this is an artifact from then.

Your patch looks fine; can you submit it to gerrit ? =)

Beyond that - this deletion listener stuff is a bit of a nightmare, and is (now) un-necessary.

Instead we can hold a VclPtr<> reference on the item, and then check if it is disposed after the call instead =)

It would be wonderful to start going through the code removing cases where we use deletion listeners =)

Would that be interesting to you ? =)

Thanks !
Comment 8 Dipankar Niranjan 2016-01-04 14:01:38 UTC
Umm,
Since you wanted to stop using deletion listeners, I haven't submitted a patch yet..

I would be interested to look into what you suggest.
Please look into this and see if you could help with filling in the gaps and tell me what I have assumed is correct
http://pastebin.com/gdWzhGXc

And I would need your advice on whether to submit a patch with the "constify" done i.e. the patch from the previous comment (I may not be able to submit any patches for the next couple of days as I'm having trouble sshing to gerrit and am in the process of resolving it)
or to complete whatever you suggest and then submit a patch as a whole.

Thanks !
Comment 9 Michael Meeks 2016-01-04 15:38:53 UTC
Hi there,

> Since you wanted to stop using deletion listeners, I haven't submitted
> a patch yet..

Lets get your initial pastebin'd patch included for now =) then we can work on killing this method separately.

Will setup a new bug for removing the deletion listeners with a good description in it.

Thanks !
Comment 10 Dipankar Niranjan 2016-01-05 08:32:51 UTC
Hi,
I've submitted the small patch.
Please review when free.
And could you possibly notify when you setup that new bug..
Like I said, I'm interested.
Thanks..
Comment 11 Michael Meeks 2016-01-05 10:27:13 UTC
Thanks for that - good work; I've merged your commit. I also added this to 'see also' - but here is a link =) it would be great to have help & testing of a series of small patches to solve that bug:

https://bugs.documentfoundation.org/show_bug.cgi?id=96888

Thanks ! =)
Comment 12 Commit Notification 2016-01-05 10:28:12 UTC
Ras-al-Ghul committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c29a6e1541f57d2a2ba8372af30e65a2a7b32a9f

tdf#89382 - vcl: unwind 'TODO' comment

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.