> On heinä 1, 2015, 10:35 ip, Aleix Pol Gonzalez wrote: > > src/widgets/ktextedit.cpp, line 662 > > <https://git.reviewboard.kde.org/r/124226/diff/1/?file=382142#file382142line662> > > > > no need for the if, you can just call `delete decorator->highlighter();` > > > > Also this is weird, deleting a returned attribute is not common, maybe > > KTextDecorator should delete it?
Ok, will remove the if. I agree that the API would be cleaner if SpellCheckDecorator had ownership of the highlighter set to it. But should that be changed and would that potentially break applications? Or did you mean to delete the default highlighter in KTextDecorator's constructor after it has been created in SpellCheckDecorator's constructor? Actually the first fix to this bug in Lokalize was http://commits.kde.org/sonnet/5ed61b10f8f733a505bf04ce82f41d5616a21f25, which removed initialization of the default highlighter in SpellCheckDecorator's constructor. That was then reverted in http://commits.kde.org/sonnet/20bb0e90ad72bb3c2d8a009c50680eeb07c6d9c7. Sounds like that removal broke something. - Lasse ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124226/#review81973 ----------------------------------------------------------- On heinä 1, 2015, 7:02 ip, Lasse Liehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124226/ > ----------------------------------------------------------- > > (Updated heinä 1, 2015, 7:02 ip) > > > Review request for KDE Frameworks and Laurent Montel. > > > Repository: ktextwidgets > > > Description > ------- > > According to SpellCheckDecorator::setHighlighter's documentation, the old > automatically created default highlighter must be manually deleted. > > This fixes bug 343459 in Lokalize. > > > Diffs > ----- > > src/widgets/ktextedit.cpp e4bd41d > > Diff: https://git.reviewboard.kde.org/r/124226/diff/ > > > Testing > ------- > > Tested that it works in Lokalize. Also ran tests. > > > Thanks, > > Lasse Liehu > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
