Re: [Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
On 2 December 2010 01:31, Lubos Lunak wrote: > On Wednesday 01 of December 2010, Jan Holesovsky wrote: >> > There appears to be a helper class for this, SwCrsrSaveState. Except >> > that it >> > doesn't seem to work, as far as I can judge SwCursor::RestoreState() >> > doesn't >> > really restore anything (quite hard to say, so much for "trivial APIs >> > don't >> > need docs"). >> >> Yeah - I tried to use that one instead when testing the patch, but >> noticed the same thing, so I just pushed the patch verbatim :-) Do we >> actually use SwCrsrSaveState somewhere (ie. is it worth fixing/removing >> for good/anything else)? [Actually, I wouldn't be surprised if there >> was a copy somewhere else that works ;-)] > > I've figured it out. SwCrsrSaveState only saves the state on the stack, the > actual restoring is done by SwCursor::RestoreSavePos(). I'll change the code > to use it. Cool, I'll be interested to see how you go with this. The first problem I ran into was that the place I added my code was place furthest down the "Search/Replace" calls I could go without the cursor position information being cleared and lost. At this stage you're executing code inside the SwView class, and only have easy access to pointers to SwWrtShell and SwCrsrShell objects, rather than the SwShellCrsr object (this naming, incidentally, caused me several hours of head banging until I figured out there were two almost-identically named classes :-P ). The reason this matters is that SwShellCrsr::RestoreSavePos() is a public method in SwShellCrsr, but SwShellCrsr::SaveState () is a *protected* method. Consequently I didn't see any easy method of gaining access to the SaveState method to actually save the state. A few calls further down the Search/Replace call stack you have access to it, but by then the cursor info has been lost. So I found that saving the cursor state needed a bit of contorting, and then making sure later calls in the Search/Replace hierarchy didn't totally blow away and reset the cursor stack where the position was saved (which it seems to be really keen to do) required even more code, and it was starting to look really ugly. Which is why I went with the two-line solution. Of course, having written that, I freely admit I'm still (somewhat blindly) feeling my way around the LO code, and you'll probably find some really elegant way to do it with a couple of lines of code, and I'll look silly :-P ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
On Wednesday 01 of December 2010, Sebastian Spaeth wrote: > On Wed, 1 Dec 2010 15:31:56 +0100, Lubos Lunak wrote: > > I've figured it out. SwCrsrSaveState only saves the state on the stack, > > the actual restoring is done by SwCursor::RestoreSavePos(). I'll change > > the code to use it. > > While you are understanding the code, can you add some comments to the > functions so it is clear how they work? That would be very helpful :). Done, of course, after the undocumented classes discussion. Do we still need to continue that one :) ? -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
On Wed, 1 Dec 2010 15:31:56 +0100, Lubos Lunak wrote: > I've figured it out. SwCrsrSaveState only saves the state on the stack, the > actual restoring is done by SwCursor::RestoreSavePos(). I'll change the code > to use it. While you are understanding the code, can you add some comments to the functions so it is clear how they work? That would be very helpful :). Sebastian pgpBNyB5zdtdp.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
On Wednesday 01 of December 2010, Jan Holesovsky wrote: > > There appears to be a helper class for this, SwCrsrSaveState. Except > > that it > > doesn't seem to work, as far as I can judge SwCursor::RestoreState() > > doesn't > > really restore anything (quite hard to say, so much for "trivial APIs > > don't > > need docs"). > > Yeah - I tried to use that one instead when testing the patch, but > noticed the same thing, so I just pushed the patch verbatim :-) Do we > actually use SwCrsrSaveState somewhere (ie. is it worth fixing/removing > for good/anything else)? [Actually, I wouldn't be surprised if there > was a copy somewhere else that works ;-)] I've figured it out. SwCrsrSaveState only saves the state on the stack, the actual restoring is done by SwCursor::RestoreSavePos(). I'll change the code to use it. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
Hi Lubos, Lubos Lunak píše v St 01. 12. 2010 v 15:05 +0100: > > > Also one of the easy hacks. > > > > > > Patch ensures that after doing "replace all" the cursor is left at > > > original position, rather than moved to the position of the last > > > replacement. > > > > Wow! - what a great usability improvement, with few lines of code > :-) > > Pushed, the only change was using the 8288 as the bug number, > instead of > > 12345. > > There appears to be a helper class for this, SwCrsrSaveState. Except > that it > doesn't seem to work, as far as I can judge SwCursor::RestoreState() > doesn't > really restore anything (quite hard to say, so much for "trivial APIs > don't > need docs"). Yeah - I tried to use that one instead when testing the patch, but noticed the same thing, so I just pushed the patch verbatim :-) Do we actually use SwCrsrSaveState somewhere (ie. is it worth fixing/removing for good/anything else)? [Actually, I wouldn't be surprised if there was a copy somewhere else that works ;-)] Regards, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
On 2 December 2010 01:05, Lubos Lunak wrote: > On Wednesday 01 of December 2010, Jan Holesovsky wrote: >> Hi Mattias, >> >> Mattias Johnsson píše v Ne 28. 11. 2010 v 14:40 +1100: >> > Also one of the easy hacks. >> > >> > Patch ensures that after doing "replace all" the cursor is left at >> > original position, rather than moved to the position of the last >> > replacement. >> >> Wow! - what a great usability improvement, with few lines of code :-) >> Pushed, the only change was using the 8288 as the bug number, instead of >> 12345. > > There appears to be a helper class for this, SwCrsrSaveState. Except that it > doesn't seem to work, as far as I can judge SwCursor::RestoreState() doesn't > really restore anything (quite hard to say, so much for "trivial APIs don't > need docs"). Yep, I know. That was one of the first things I tried. No luck. Or, rather, it does save some state, and push it onto the cursor stack, but it's erased again later. Rather than making major invasive changes to the code, I went for the simple option. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
On Wednesday 01 of December 2010, Jan Holesovsky wrote: > Hi Mattias, > > Mattias Johnsson píše v Ne 28. 11. 2010 v 14:40 +1100: > > Also one of the easy hacks. > > > > Patch ensures that after doing "replace all" the cursor is left at > > original position, rather than moved to the position of the last > > replacement. > > Wow! - what a great usability improvement, with few lines of code :-) > Pushed, the only change was using the 8288 as the bug number, instead of > 12345. There appears to be a helper class for this, SwCrsrSaveState. Except that it doesn't seem to work, as far as I can judge SwCursor::RestoreState() doesn't really restore anything (quite hard to say, so much for "trivial APIs don't need docs"). -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
On 2 December 2010 00:30, Jan Holesovsky wrote: > Hi Mattias, > > Mattias Johnsson píše v Ne 28. 11. 2010 v 14:40 +1100: > >> Also one of the easy hacks. >> >> Patch ensures that after doing "replace all" the cursor is left at >> original position, rather than moved to the position of the last >> replacement. > > Wow! - what a great usability improvement, with few lines of code :-) > Pushed, the only change was using the 8288 as the bug number, instead of > 12345. > > Can you please move it from Easy Hacks to Completed Easy Hacks? ;-) > > http://wiki.documentfoundation.org/Development/Easy_Hacks/Completed > Thanks, moved :-) ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] [PUSHED] Re: [PATCH] Fix for bug/feature request i#8288
Hi Mattias, Mattias Johnsson píše v Ne 28. 11. 2010 v 14:40 +1100: > Also one of the easy hacks. > > Patch ensures that after doing "replace all" the cursor is left at > original position, rather than moved to the position of the last > replacement. Wow! - what a great usability improvement, with few lines of code :-) Pushed, the only change was using the 8288 as the bug number, instead of 12345. Can you please move it from Easy Hacks to Completed Easy Hacks? ;-) http://wiki.documentfoundation.org/Development/Easy_Hacks/Completed Thank you a lot, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
