Re: Scrollfix patch
Abdelrazak Younes ha scritto: GuiApplication). On a related note we have WorkArea::scheduleRedraw() that was created for the purpose of delayed screen updates (scheduled to happen at next cursor blink). I know. Actually, I cannot imagine a use-case in which delaying screen redraw to next cursor blinking is really useful, probably you can shed some light. I'm scared that, if the cursor is not blinking because it is not visible, then you miss the redraw. If you need delayed redraw, maybe a timer would suit your needs, wouldn't it ? In my case of parHeights updates, I need something that is as fast as possible without disrupting user operation, i.e. smth. that gives priority to user operations and requests w.r.t. my background activity, and the singleShot() semantics is perfectly what I need. I guess you can something similar for the scrollbar updates. In general I prefer explicit rather than implicit. Please, explain explicit vs implicit, thanks, bye, T.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Abdelrazak Younes ha scritto: GuiApplication). On a related note we have WorkArea::scheduleRedraw() that was created for the purpose of delayed screen updates (scheduled to happen at next cursor blink). I know. Actually, I cannot imagine a use-case in which delaying screen redraw to next cursor blinking is really useful, probably you can shed some light. I'm scared that, if the cursor is not blinking because it is not visible, then you miss the redraw. If you need delayed redraw, maybe a timer would suit your needs, wouldn't it ? Not really. We use cursor blinking so as to not bother the user too much when a graphic preview is generated in the background. Formerly (in 1.4.x), a redraw occurred as soon as an image or an equation was converted for display. For some users that use the XYpic package, that was very problematic because LyX remained frozen for a long time at first load. With the scheduleRedraw() solution, we don't redraw immediately but at the next cursor blink, so that multiple redraw requests are merged into a single one. In my case of parHeights updates, I need something that is as fast as possible without disrupting user operation, i.e. smth. that gives priority to user operations and requests w.r.t. my background activity, and the singleShot() semantics is perfectly what I need. You are probably right. I guess you can something similar for the scrollbar updates. In general I prefer explicit rather than implicit. Please, explain explicit vs implicit, thanks, In general I prefer a method that explicitly describes what it does rather than a too generic template based solution. Abdel.
Re: Scrollfix patch
Abdelrazak Younes ha scritto: GuiApplication). On a related note we have WorkArea::scheduleRedraw() that was created for the purpose of delayed screen updates (scheduled to happen at next cursor blink). I know. Actually, I cannot imagine a use-case in which delaying screen redraw to next cursor blinking is really useful, probably you can shed some light. I'm scared that, if the cursor is not blinking because it is not visible, then you miss the redraw. If you need delayed redraw, maybe a timer would suit your needs, wouldn't it ? In my case of parHeights updates, I need something that is as fast as possible without disrupting user operation, i.e. smth. that gives priority to user operations and requests w.r.t. my background activity, and the singleShot() semantics is perfectly what I need. I guess you can something similar for the scrollbar updates. In general I prefer explicit rather than implicit. Please, explain explicit vs implicit, thanks, bye, T.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Abdelrazak Younes ha scritto: GuiApplication). On a related note we have WorkArea::scheduleRedraw() that was created for the purpose of delayed screen updates (scheduled to happen at next cursor blink). I know. Actually, I cannot imagine a use-case in which delaying screen redraw to next cursor blinking is really useful, probably you can shed some light. I'm scared that, if the cursor is not blinking because it is not visible, then you miss the redraw. If you need delayed redraw, maybe a timer would suit your needs, wouldn't it ? Not really. We use cursor blinking so as to not bother the user too much when a graphic preview is generated in the background. Formerly (in 1.4.x), a redraw occurred as soon as an image or an equation was converted for display. For some users that use the XYpic package, that was very problematic because LyX remained frozen for a long time at first load. With the scheduleRedraw() solution, we don't redraw immediately but at the next cursor blink, so that multiple redraw requests are merged into a single one. In my case of parHeights updates, I need something that is as fast as possible without disrupting user operation, i.e. smth. that gives priority to user operations and requests w.r.t. my background activity, and the singleShot() semantics is perfectly what I need. You are probably right. I guess you can something similar for the scrollbar updates. In general I prefer explicit rather than implicit. Please, explain explicit vs implicit, thanks, In general I prefer a method that explicitly describes what it does rather than a too generic template based solution. Abdel.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Helge Hafting ha scritto: He offered to do the calculation in the background. That gives the I'm already on the way ... almost working, but need further debugging. After that, guess my last change will be to make Abdel happy and confine the nice-scrolling behaviour within the BufferView class, so to avoid too many changes to TextMetrics (and the ParMetricsCache class). Hi Tommaso, While I am happy that you want to make me happy please don't do that only for that. I certainly hope that I convinced you but I don't want to force you on anything. On a related note, in order to call a method when the GUI is idle, I'm using the QTimer::singleShot() method, with a delta_time of 0. Do you think that would turn to be useful for other things as well ? I mean, something like GuiView::doWhenIdle(this, myMethod): templateclass T static GuiView::doWhenIdle(T *, bool (T::*)(void)) I suggest to implement this if there is a second need arising for that (and I would not place that in GuiView but in GuiApplication). On a related note we have WorkArea::scheduleRedraw() that was created for the purpose of delayed screen updates (scheduled to happen at next cursor blink). I guess you can something similar for the scrollbar updates. In general I prefer explicit rather than implicit. Abdel.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Helge Hafting ha scritto: He offered to do the calculation in the background. That gives the I'm already on the way ... almost working, but need further debugging. After that, guess my last change will be to make Abdel happy and confine the nice-scrolling behaviour within the BufferView class, so to avoid too many changes to TextMetrics (and the ParMetricsCache class). Hi Tommaso, While I am happy that you want to make me happy please don't do that only for that. I certainly hope that I convinced you but I don't want to force you on anything. On a related note, in order to call a method when the GUI is "idle", I'm using the QTimer::singleShot() method, with a delta_time of 0. Do you think that would turn to be useful for other things as well ? I mean, something like GuiView::doWhenIdle(this, ): template static GuiView::doWhenIdle(T *, bool (T::*)(void)) I suggest to implement this if there is a second need arising for that (and I would not place that in GuiView but in GuiApplication). On a related note we have WorkArea::scheduleRedraw() that was created for the purpose of delayed screen updates (scheduled to happen at next cursor blink). I guess you can something similar for the scrollbar updates. In general I prefer explicit rather than implicit. Abdel.
Re: Scrollfix patch
Abdelrazak Younes wrote: Andre Poenitz wrote: On Sun, Nov 11, 2007 at 04:39:22PM +0100, Tommaso Cucinotta wrote: If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a summing (balanced) tree, that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). I think loading/resizing in 4 second is ok. I disagree with that. Loading should be as fast as possible and resizing should be instantaneous. Or we just switch to implement a WISIWIG processor. There are a lot of good things in Tommaso's patch but the initial calculation of all ParMetrics is not one of those. The TextMetrics API additions are nice but are orthogonal to our scrolling problem. 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... That's the thing I hate most about MSWords: continuously waiting for the re-pagination to complete. We don't need a *perfect* scrolling behaviour, just a sensible one. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. He offered to do the calculation in the background. That gives the best of both worlds - You can start editing as soon as the .lyx file is parsed. You can scroll immediately, and the scrolling will be perfect after a few seconds. Speed sure is nice, but the current scrolling that breaks on common corner cases is not good. Helge Hafting
Re: Scrollfix patch
Helge Hafting ha scritto: He offered to do the calculation in the background. That gives the I'm already on the way ... almost working, but need further debugging. After that, guess my last change will be to make Abdel happy and confine the nice-scrolling behaviour within the BufferView class, so to avoid too many changes to TextMetrics (and the ParMetricsCache class). On a related note, in order to call a method when the GUI is idle, I'm using the QTimer::singleShot() method, with a delta_time of 0. Do you think that would turn to be useful for other things as well ? I mean, something like GuiView::doWhenIdle(this, myMethod): templateclass T static GuiView::doWhenIdle(T *, bool (T::*)(void)) (with something like myMethod() returning true or false meaning call me again or not). Any drawbacks with such an approach (I know there is a support/timer.h class, but guess it cannot know when the GUI is idle). T. best of both worlds - You can start editing as soon as the .lyx file is parsed. You can scroll immediately, and the scrolling will be perfect after a few seconds. Speed sure is nice, but the current scrolling that breaks on common corner cases is not good. Helge Hafting
Re: Scrollfix patch
On Mon, Nov 12, 2007 at 08:11:38PM +0100, Tommaso Cucinotta wrote: Helge Hafting ha scritto: He offered to do the calculation in the background. That gives the I'm already on the way ... almost working, but need further debugging. After that, guess my last change will be to make Abdel happy and confine the nice-scrolling behaviour within the BufferView class, so to avoid too many changes to TextMetrics (and the ParMetricsCache class). On a related note, in order to call a method when the GUI is idle, I'm using the QTimer::singleShot() method, with a delta_time of 0. There's also bool QCoreApplication::hasPendingEvents(). The visible effect should be more or less ok. Do you think that would turn to be useful for other things as well ? I mean, something like GuiView::doWhenIdle(this, myMethod): templateclass T static GuiView::doWhenIdle(T *, bool (T::*)(void)) (with something like myMethod() returning true or false meaning call me again or not). [I am not sure about the utility of this approach. No need to proactively introduce funny template constructs ;-}] Any drawbacks with such an approach (I know there is a support/timer.h class, Yes, that's the thirteenth wheel of our cart... but guess it cannot know when the GUI is idle). Andre'
Re: Scrollfix patch
Andre Poenitz ha scritto: I mean, something like GuiView::doWhenIdle(this, myMethod): templateclass T static GuiView::doWhenIdle(T *, bool (T::*)(void)) [I am not sure about the utility of this approach. No need to proactively introduce funny template constructs ;-}] The utility comes if the mechanism is potentially useful for other classes, for other purposes (not only for BufferView for my specific purpose). Any class that needs to be called back could use it with a simple call like the one of above, confined in the implementation file (basically it's a functor approach, and, yes, probably there's smth. in STL that does that). The alternative to templates, of course, is introduction of a common base interface (class), what is also invasive in that it forces the user class to inherit from the interface. T. Any drawbacks with such an approach (I know there is a support/timer.h class, Yes, that's the thirteenth wheel of our cart... but guess it cannot know when the GUI is idle). Andre'
Re: Scrollfix patch
Abdelrazak Younes wrote: Andre Poenitz wrote: On Sun, Nov 11, 2007 at 04:39:22PM +0100, Tommaso Cucinotta wrote: If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a "summing (balanced) tree", that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). I think loading/resizing in 4 second is ok. I disagree with that. Loading should be as fast as possible and resizing should be instantaneous. Or we just switch to implement a WISIWIG processor. There are a lot of good things in Tommaso's patch but the initial calculation of all ParMetrics is not one of those. The TextMetrics API additions are nice but are orthogonal to our scrolling problem. 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... That's the thing I hate most about MSWords: continuously waiting for the re-pagination to complete. We don't need a *perfect* scrolling behaviour, just a sensible one. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. He offered to do the calculation in the background. That gives the best of both worlds - You can start editing as soon as the .lyx file is parsed. You can scroll immediately, and the scrolling will be "perfect" after a few seconds. Speed sure is nice, but the current scrolling that breaks on common corner cases is not good. Helge Hafting
Re: Scrollfix patch
Helge Hafting ha scritto: He offered to do the calculation in the background. That gives the I'm already on the way ... almost working, but need further debugging. After that, guess my last change will be to make Abdel happy and confine the nice-scrolling behaviour within the BufferView class, so to avoid too many changes to TextMetrics (and the ParMetricsCache class). On a related note, in order to call a method when the GUI is "idle", I'm using the QTimer::singleShot() method, with a delta_time of 0. Do you think that would turn to be useful for other things as well ? I mean, something like GuiView::doWhenIdle(this, ): template static GuiView::doWhenIdle(T *, bool (T::*)(void)) (with something like myMethod() returning true or false meaning call me again or not). Any drawbacks with such an approach (I know there is a support/timer.h class, but guess it cannot know when the GUI is idle). T. best of both worlds - You can start editing as soon as the .lyx file is parsed. You can scroll immediately, and the scrolling will be "perfect" after a few seconds. Speed sure is nice, but the current scrolling that breaks on common corner cases is not good. Helge Hafting
Re: Scrollfix patch
On Mon, Nov 12, 2007 at 08:11:38PM +0100, Tommaso Cucinotta wrote: > Helge Hafting ha scritto: >> He offered to do the calculation in the background. That gives the > I'm already on the way ... almost working, but need further debugging. > After that, guess my last change will be to make Abdel happy and > confine the nice-scrolling behaviour within the BufferView class, so to > avoid too many changes to TextMetrics (and the ParMetricsCache class). > > On a related note, in order to call a method when the GUI is "idle", > I'm using the QTimer::singleShot() method, with a delta_time of 0. There's also bool QCoreApplication::hasPendingEvents(). The visible effect should be more or less ok. > Do you think that would turn to be useful for other things as well ? > I mean, something like GuiView::doWhenIdle(this, ): > > template static GuiView::doWhenIdle(T *, bool (T::*)(void)) > > (with something like myMethod() returning true or false meaning call > me again or not). [I am not sure about the utility of this approach. No need to proactively introduce funny template constructs ;-}] > Any drawbacks with such an approach (I know there > is a support/timer.h class, Yes, that's the thirteenth wheel of our cart... > but guess it cannot know when the GUI is idle). Andre'
Re: Scrollfix patch
Andre Poenitz ha scritto: I mean, something like GuiView::doWhenIdle(this, ): template static GuiView::doWhenIdle(T *, bool (T::*)(void)) [I am not sure about the utility of this approach. No need to proactively introduce funny template constructs ;-}] The utility comes if the mechanism is potentially useful for other classes, for other purposes (not only for BufferView for my specific purpose). Any class that needs to be called back could use it with a simple call like the one of above, confined in the implementation file (basically it's a functor approach, and, yes, probably there's smth. in STL that does that). The alternative to templates, of course, is introduction of a common base interface (class), what is also "invasive" in that it forces the user class to inherit from the interface. T. Any drawbacks with such an approach (I know there is a support/timer.h class, Yes, that's the thirteenth wheel of our cart... but guess it cannot know when the GUI is idle). Andre'
Re: Scrollfix patch
-) the boxes example now works correctly, except it is somewhat slow because with SinglePar I'm updating and redrawing the outer Par, not the inner one, so with such a great box (or even table, etc...), it gets slow -- here I should add a further parameter to UpdateScope specifying the Text where the update starts from; yes, scrolling is ok and typing is slow. moreover this patch breaks pg/up-down inside insets - which is slightly broken even now as noted in regression list. (by this i dont mean it shouldnt go in, as i suppose you'll work on the bugs further). pavel
Re: Scrollfix patch
Martin Vermeer wrote: On Sun, Nov 11, 2007 at 05:24:36AM +0100, Tommaso Cucinotta wrote: As it is somewhat growing, I wouldn't like to keep working on it for too much time further, as the operation of merging with trunk may become cumbersome. Now it seems to me sufficiently stable. T. I think this is the time to check it in -- with the trivial renames you propose. I hope you mean _without_ the trivial renames... We're not close to a trunk release so any bugs will get ironed out in a timely manner. I'd prefer that the patch is incrementally integrated. This patch should serve as a reference goal implementation. I am talking about how I would personally proceed but I am not Tommaso. It this patch goes as-is you have to promise to fixed all bugs and regressions Tommaso (I would help you of course). I'll to do a review of the patch in the coming days. Abdel.
Re: Scrollfix patch
Abdelrazak Younes wrote: It this patch goes as-is you have to promise to fixed all bugs and regressions Tommaso (I would help you of course). If this patch goes in as-is you have to promise to fix all bugs and regressions Tommaso (I would help you of course). I'll to do a review of the patch in the coming days. I'll try to do a review of the patch in the coming days. Looking at the state of my spelling, obviously not today :-) Abdel.
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 07:49:54AM +0200, Martin Vermeer wrote: On Sun, Nov 11, 2007 at 05:24:36AM +0100, Tommaso Cucinotta wrote: Hi all, please, find attached an improved version of the scrollfix patch. Summary of the further changes: -) most optimizations for updating a single par are back -) actually, it is possible to specify a par range with Update::SinglePar -- actually, should be renamed as Update::Paragraphs; -- actually, Update::Force should be renamed as Update::Screen ? -) class UpdateScope encapsulates the scope of an update (updateFlags plus parameters, e.g. the par range to be updated if SinglePar is used) -) the boxes example now works correctly, except it is somewhat slow because with SinglePar I'm updating and redrawing the outer Par, not the inner one, so with such a great box (or even table, etc...), it gets slow -- here I should add a further parameter to UpdateScope specifying the Text where the update starts from; -) probably it adheres slightly more to the coding rules; -) updated to trunk revision 21533; -) needs testing to check if there are any further crashes; As it is somewhat growing, I wouldn't like to keep working on it for too much time further, as the operation of merging with trunk may become cumbersome. Now it seems to me sufficiently stable. T. I think this is the time to check it in -- with the trivial renames you propose. We're not close to a trunk release so any bugs will get ironed out in a timely manner. I'd like to see some performance measures first, lest we paint ourselves into a corner here. We had this kind of approach (all paragraph heights known) for a long time and switched to the current one for performance reasons when e.g. loading/resizing inserting in big docments. If we blindly apply fundamental changes to the paint strategy it might well be that there are some problems with the strategy that are _conceptually_ not solvable. Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 01:21:03PM +0100, Andre Poenitz wrote: I think this is the time to check it in -- with the trivial renames you propose. We're not close to a trunk release so any bugs will get ironed out in a timely manner. I'd like to see some performance measures first, lest we paint ourselves into a corner here. An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 01:53:52PM +0100, Andre Poenitz wrote: On Sun, Nov 11, 2007 at 01:21:03PM +0100, Andre Poenitz wrote: I think this is the time to check it in -- with the trivial renames you propose. We're not close to a trunk release so any bugs will get ironed out in a timely manner. I'd like to see some performance measures first, lest we paint ourselves into a corner here. An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. Looks better with the debug output disabled. Still: So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Also, loading the UserGuide and keeping PageDown pressed gets stuck at some point of time. Also, on the way there the cursor is sometimes off-screen. Maybe you can also get in touch with Alfredo. He spent quite some time with the current architecture so he might be aware of more pitfalls than I am. Andre'
Re: Scrollfix patch
Andre Poenitz ha scritto: ourselves into a corner here. We had this kind of approach (all paragraph heights known) for a long time and switched to the current one for performance reasons when e.g. loading/resizing inserting in big docments. My feeling is that, even with hundreds of outer paragraphs, the process of summing their heights up should not be even perceivable on modern CPUs (provided that the heights are directly available within a vector, like it is now -- ParMetricsCache is not a map anymore, but a vector). And, more importantly, such process would be triggered only as a response to: 1) change of height of currently edited par 1.a) break of currently edited par (Enter) 2) cut and paste operations 3) width resize of screen If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a summing (balanced) tree, that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). If you look at the patch code, you'll notice that many times (not all, though) paragraph heights are managed through dedicated methods (tm.parHeight(), tm.computeHeight(par1, par2)), instead of using the standard par_metrics_[p].height(), just in view of such possibility. T.
Re: Scrollfix patch
Andre Poenitz ha scritto: An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. Thanx, I'll try to figure it out... Looks better with the debug output disabled. Still: Did you get the same numbers with debug output disabled ? T.
Re: Scrollfix patch
Andre Poenitz ha scritto: An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Well, I'm verifying on my laptop. But I want to be sure to have two LyX compiled exactly with the same options, and debug disabled. On my laptop ([EMAIL PROTECTED]), it takes ~30min. a full recompilation of LyX, so I guess I'll have numbers within one hour or so (and I'm also deleting a couple of kernel source trees to accomodate for the needed space). Anyway, please note that, with the current patch, *all* par metrics and heights are pre-computed on a document load (full metrics are discarded for all pars but the visible ones), so I expect anycase a slow-down on document opening. As already mentioned on the list, I'd like to switch to an incremental computation of the document height that is made in the background while the user is already able to work on the document. I'd make such change as a further incremental patch, but if you prefer the all-in-one approach, I can go further to such step. T.
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 04:39:22PM +0100, Tommaso Cucinotta wrote: Andre Poenitz ha scritto: ourselves into a corner here. We had this kind of approach (all paragraph heights known) for a long time and switched to the current one for performance reasons when e.g. loading/resizing inserting in big docments. My feeling is that, even with hundreds of outer paragraphs, the process of summing their heights up should not be even perceivable on modern CPUs (provided that the heights are directly available within a vector, like it is now -- ParMetricsCache is not a map anymore, but a vector). And, more importantly, such process would be triggered only as a response to: 1) change of height of currently edited par 1.a) break of currently edited par (Enter) 2) cut and paste operations 3) width resize of screen 4) Loading of a document. If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a summing (balanced) tree, that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). I think loading/resizing in 4 second is ok. 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... If you look at the patch code, you'll notice that many times (not all, though) paragraph heights are managed through dedicated methods (tm.parHeight(), tm.computeHeight(par1, par2)), instead of using the standard par_metrics_[p].height(), just in view of such possibility. The patch as such looks like a sensible implementation of the idea (plus/minus some minor quirks). However, as this is quite intrusive change of concept I'd rather make sure the concept is sound. And I do not really want to rely on gut feelings only here (my gut says: Id shoul be ok as we used to o it like that for a while and the lazy rebreaking was not too much of a success and machines are faster tofday then five years ago). Andre'
Re: Scrollfix patch
Andre Poenitz wrote: On Sun, Nov 11, 2007 at 04:39:22PM +0100, Tommaso Cucinotta wrote: If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a summing (balanced) tree, that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). I think loading/resizing in 4 second is ok. I disagree with that. Loading should be as fast as possible and resizing should be instantaneous. Or we just switch to implement a WISIWIG processor. There are a lot of good things in Tommaso's patch but the initial calculation of all ParMetrics is not one of those. The TextMetrics API additions are nice but are orthogonal to our scrolling problem. 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... That's the thing I hate most about MSWords: continuously waiting for the re-pagination to complete. We don't need a *perfect* scrolling behaviour, just a sensible one. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. Abdel.
Re: Scrollfix patch
Andre Poenitz wrote: 1) change of height of currently edited par 1.a) break of currently edited par (Enter) 2) cut and paste operations 3) width resize of screen 4) Loading of a document. If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a summing (balanced) tree, that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). I think loading/resizing in 4 second is ok. 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... Mmm... I think this would be pretty bad. If you look at the patch code, you'll notice that many times (not all, though) paragraph heights are managed through dedicated methods (tm.parHeight(), tm.computeHeight(par1, par2)), instead of using the standard par_metrics_[p].height(), just in view of such possibility. The patch as such looks like a sensible implementation of the idea (plus/minus some minor quirks). However, as this is quite intrusive change of concept I'd rather make sure the concept is sound. And I do not really want to rely on gut feelings only here (my gut says: Id shoul be ok as we used to o it like that for a while and the lazy rebreaking was not too much of a success and machines are faster tofday then five years ago). My main concern is that is much more difficult to go the nonlazy - lazy route as we have done (and Andre' and myself invested a good deal of time doing it) than the other way around. So we should make this decision very carefully. PS: I *think* I agree with Andre on this point, but I rather not say it 'cause lately I'm not guessing right ;-) A/
Re: Scrollfix patch
Abdelrazak Younes wrote: If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a summing (balanced) tree, that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). I think loading/resizing in 4 second is ok. I disagree with that. Loading should be as fast as possible and resizing should be instantaneous. Exactly Or we just switch to implement a WISIWIG processor. There are a lot of good things in Tommaso's patch but the initial calculation of all ParMetrics is not one of those. The TextMetrics API additions are nice but are orthogonal to our scrolling problem. 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... That's the thing I hate most about MSWords: continuously waiting for the re-pagination to complete. We don't need a *perfect* scrolling behaviour, just a sensible one. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. I agree 100%. A/
Re: Scrollfix patch
Andre Poenitz wrote: On Sun, Nov 11, 2007 at 01:21:03PM +0100, Andre Poenitz wrote: I think this is the time to check it in -- with the trivial renames you propose. We're not close to a trunk release so any bugs will get ironed out in a timely manner. I'd like to see some performance measures first, lest we paint ourselves into a corner here. An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. A small point: when testing we should not limit ourselves to the Userguide. IMO one of the really strong points of LyX is to be able to deal with huge documents, like books. (the UG has just 150 pages, it is like a small book). Btw, we used to test performance with UG20 (just 20 copies of UserGuide) some time (but I agree that this is kind of extreme, though). A/
Re: Scrollfix patch
Tommaso Cucinotta wrote: Andre Poenitz ha scritto: ourselves into a corner here. We had this kind of approach (all paragraph heights known) for a long time and switched to the current one for performance reasons when e.g. loading/resizing inserting in big docments. My feeling is that, even with hundreds of outer paragraphs, the process of summing their heights up should not be even perceivable on modern CPUs (provided that the heights are directly available within a vector, like it is now -- ParMetricsCache is not a map anymore, but a vector). And, more importantly, such process would be triggered only as a response to: 1) change of height of currently edited par 1.a) break of currently edited par (Enter) 2) cut and paste operations 3) width resize of screen 3) document loading 4) graphics updating - this is particular important with math previews on a document full of them. If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a summing (balanced) tree, that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). This could be a good idea (if needed of course, numbers should tell). A/
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 05:25:20PM +0100, Tommaso Cucinotta wrote: Andre Poenitz ha scritto: An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Well, I'm verifying on my laptop. But I want to be sure to have two LyX compiled exactly with the same options, and debug disabled. On my laptop ([EMAIL PROTECTED]), it takes ~30min. a full recompilation of LyX, so I guess I'll have numbers within one hour or so (and I'm also deleting a couple of kernel source trees to accomodate for the needed space). Anyway, please note that, with the current patch, *all* par metrics and heights are pre-computed on a document load (full metrics are discarded for all pars but the visible ones), so I expect anycase a slow-down on document opening. Some performance decrease for loading will be acceptable... As already mentioned on the list, I'd like to switch to an incremental computation of the document height that is made in the background while the user is already able to work on the document. I'd make such change as a further incremental patch, but if you prefer the all-in-one approach, I can go further to such step. Incremental is fine if the direction is ok and each step leaves us with a usable code base. Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 06:21:45PM +0100, Abdelrazak Younes wrote: 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... That's the thing I hate most about MSWords: continuously waiting for the re-pagination to complete. We don't need a *perfect* scrolling behaviour, just a sensible one. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. So looks like Tommasso and you will need to talk to each other ;-) Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 06:46:38PM +0100, Alfredo Braunstein wrote: Andre Poenitz wrote: On Sun, Nov 11, 2007 at 01:21:03PM +0100, Andre Poenitz wrote: I think this is the time to check it in -- with the trivial renames you propose. We're not close to a trunk release so any bugs will get ironed out in a timely manner. I'd like to see some performance measures first, lest we paint ourselves into a corner here. An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. A small point: when testing we should not limit ourselves to the Userguide. IMO one of the really strong points of LyX is to be able to deal with huge documents, like books. (the UG has just 150 pages, it is like a small book). Btw, we used to test performance with UG20 (just 20 copies of UserGuide) some time (but I agree that this is kind of extreme, though). But if a concept does not scale up well it's usually a bad concept. So checking with ug20 is certainly still a vaild approach. Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 08:08:52PM +0100, Andre Poenitz wrote: On Sun, Nov 11, 2007 at 05:25:20PM +0100, Tommaso Cucinotta wrote: Andre Poenitz ha scritto: An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Well, I'm verifying on my laptop. But I want to be sure to have two LyX compiled exactly with the same options, and debug disabled. On my laptop ([EMAIL PROTECTED]), it takes ~30min. a full recompilation of LyX, so I guess I'll have numbers within one hour or so (and I'm also deleting a couple of kernel source trees to accomodate for the needed space). Anyway, please note that, with the current patch, *all* par metrics and heights are pre-computed on a document load (full metrics are discarded for all pars but the visible ones), so I expect anycase a slow-down on document opening. Some performance decrease for loading will be acceptable... But it looks like Abdel and Alfredo disagree. So maybe some kind of discussion is needed. Andre'
Re: Scrollfix patch
Andre Poenitz wrote: On Sun, Nov 11, 2007 at 06:46:38PM +0100, Alfredo Braunstein wrote: A small point: when testing we should not limit ourselves to the Userguide. IMO one of the really strong points of LyX is to be able to deal with huge documents, like books. (the UG has just 150 pages, it is like a small book). Btw, we used to test performance with UG20 (just 20 copies of UserGuide) some time (but I agree that this is kind of extreme, though). Yes, this plus the fact that one often loads many documents at startup. But if a concept does not scale up well it's usually a bad concept. So checking with ug20 is certainly still a vaild approach. And it is always my approach when testing file loading or label/toc updates. Abdel.
Re: Scrollfix patch
Ok, I have my own numbers. This is what I obtained launching LyX from the shell asking to open a document, and hitting Alt-F4 (close application) as soon as I see the LyX window. Note that, before LyX is closed, I can see clearly the loaded document showing on the screen, then the pressed sequence closes LyX (therefore in the patched case, full par metrics and heights computation is done). Note also that I didn't report numbers from the *first* execution, but I repeated the process 3 or 4 times, then I reported the last numbers. This is done to reach a fair condition in which all files are basically cached in the Linux disk cache when launching the program. This is trunk, as checked out ~2 hour ago (USERGUIDE.LYX): [EMAIL PROTECTED]:~/lyx-trunk$ time sudo nice ./src/lyx lib/doc/UserGuide.lyx /dev/null 21 real0m3.057s user0m2.127s sys 0m0.112s This is my scrollfix patch, as sent to the list tonight: [EMAIL PROTECTED]:~/lyx-devel$ time sudo nice ./src/lyx lib/doc/UserGuide.lyx /dev/null 21 real0m2.611s user0m1.719s sys 0m0.110s And now again trunk for EMBEDDEDOBJECTS.LYX: [EMAIL PROTECTED]:~/lyx-trunk$ time sudo nice ./src/lyx lib/doc/EmbeddedObjects.lyx /dev/null 21 real0m2.887s user0m1.912s sys 0m0.141s and again my scrollfix patch: [EMAIL PROTECTED]:~/lyx-devel$ time sudo nice ./src/lyx lib/doc/EmbeddedObjects.lyx /dev/null 21 real0m3.156s user0m2.269s sys 0m0.106s Conclusions: the unscientific experiment of Andre Poenits was really non-scientific. I guess he must have left debug enabled when trying the scrollfix, and disabled in the other case :-) Another possibility could be he tried the scrollfix with an empty disk cache, then trunk with a filled disk-cache. The difference is enormous, as shown below: This is what happens to trunk if I clear the disk cache: [EMAIL PROTECTED]:~# sync echo 3 /proc/sys/vm/drop_caches [EMAIL PROTECTED]:~/lyx-trunk$ time sudo nice ./src/lyx lib/doc/EmbeddedObjects.lyx /dev/null 21 real0m18.461s user0m1.952s sys 0m0.195s and this is my scrollfix after the same process: [EMAIL PROTECTED]:~# sync echo 3 /proc/sys/vm/drop_caches [EMAIL PROTECTED]:~/lyx-devel$ time sudo nice ./src/lyx lib/doc/EmbeddedObjects.lyx /dev/null 21 real0m19.146s user0m2.268s sys 0m0.187s As you can see, ~20 seconds are needed to load LyX along with all the shared libraries it uses, configuration files, and the document itself. The time actually spent by the CPU does not change that much (~2 sec.). If anyone has a longer file (a Master thesis, PhD thesis or something like that), I can try to see what is the doc length at which things start really slowing down inaccpetably. T. Tommaso Cucinotta ha scritto: Andre Poenitz ha scritto: An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Well, I'm verifying on my laptop. But I want to be sure to have two LyX compiled exactly with the same options, and debug disabled. On my laptop ([EMAIL PROTECTED]), it takes ~30min. a full recompilation of LyX, so I guess I'll have numbers within one hour or so (and I'm also deleting a couple of kernel source trees to accomodate for the needed space). Anyway, please note that, with the current patch, *all* par metrics and heights are pre-computed on a document load (full metrics are discarded for all pars but the visible ones), so I expect anycase a slow-down on document opening. As already mentioned on the list, I'd like to switch to an incremental computation of the document height that is made in the background while the user is already able to work on the document. I'd make such change as a further incremental patch, but if you prefer the all-in-one approach, I can go further to such step. T. -- Tommaso Cucinotta, Computer Engineering PhD, Researcher ReTiS Lab, Scuola Superiore Sant'Anna, Pisa, Italy Tel +39 050 882 024, Fax +39 050 882 003 http://feanor.sssup.it/~tommaso
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 08:59:01PM +0100, Tommaso Cucinotta wrote: Conclusions: the unscientific experiment of Andre Poenits was really non-scientific. I guess he must have left debug enabled when trying the scrollfix, and disabled in the other case :-) Another possibility could be he tried the scrollfix with an empty disk cache, then trunk with a filled disk-cache. The difference is enormous, as shown below: Note that your patch created lots of output by itself. So when I test trunk vs patch I magically get more debug output. If anyone has a longer file (a Master thesis, PhD thesis or something like that), I can try to see what is the doc length at which things start really slowing down inaccpetably. Just copy the UserGuide 20 times into itself. Andre'
Re: Scrollfix patch
Abdelrazak Younes ha scritto: We don't need a *perfect* scrolling behaviour, just a sensible one. I agree we may accept a non-perfect scrolling right after having opened a document, or having made great changes. But, IMHO, after a while it would be better to have a precise behaviour, while reading you doc, searching for places to improve/change and editing. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. Well, this is actually what I did, except I'm caching also the width, the ascent and the descent, along with the height. Thought they could have turned out to be useful, someday. Basically, first I added a vector of heights, but immediately after I realized it was sufficient to reuse the ParagraphMetrics type, with the rows_ field cleared. So, the par_metrics_[] vector contains width, height, asc and desc of all *outer* paragraphs in the document, while only visible ones have the .rows() collection filled in. Well, it is not really a cache in the cache sense... T.
Re: Scrollfix patch
Andre Poenitz ha scritto: Just copy the UserGuide 20 times into itself. Done. I get a time roughly doubled: trunk: 5.5 secs scrollfix: 10.5 secs Well, I guess nobody really works with so large documents, but you have at least one file per chapter. Though, I can add the background-incremental height computation behaviour to my patch, in an attempt to make everybody happy. T.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Abdelrazak Younes ha scritto: We don't need a *perfect* scrolling behaviour, just a sensible one. I agree we may accept a non-perfect scrolling right after having opened a document, or having made great changes. But, IMHO, after a while it would be better to have a precise behaviour, while reading you doc, searching for places to improve/change and editing. I think that updating the par height the moment it is displayed on screen would be enough. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. Well, this is actually what I did, except I'm caching also the width, the ascent and the descent, along with the height. Thought they could have turned out to be useful, someday. I don't think this is useful. In any case, it is not useful now so why the memory waste? Basically, first I added a vector of heights, but immediately after I realized it was sufficient to reuse the ParagraphMetrics type, with the rows_ field cleared. I understand your reasoning but I disagree, sorry... see below. So, the par_metrics_[] vector contains width, height, asc and desc of all *outer* paragraphs in the document, while only visible ones have the .rows() collection filled in. I don't like this distinction. I prefer all or nothing. Actually I planned to have partial metrics also for non-main Insets. You are doing the other way around: full metrics for the main Inset. Well, it is not really a cache in the cache sense... Not in your patch no. Abdel.
Re: Scrollfix patch
Abdelrazak Younes ha scritto: I don't like this distinction. I prefer all or nothing. Well, if you really think it is preferreable, then it would probably be a matter of a few minutes to create a new vector TextMetrics::par_heights_[], in addition to the par_metrics_[] one, and let the parHeight() / computeHeight() methods rely on the new vector, rather than par_metrics_[]. T.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Abdelrazak Younes ha scritto: I don't like this distinction. I prefer all or nothing. Well, if you really think it is preferreable, then it would probably be a matter of a few minutes to create a new vector TextMetrics::par_heights_[], in addition to the par_metrics_[] one, Well, I don't think par_metrics_ should become a vector... I used a map to minimize memory and simplify access (no worry about resize etc). The inset metrics, the CoordCache and the Rows all behave in a consistent way. I fixed an unbelievable number of bugs thanks to this consistency. This is a simple mental model that works well now and I'd prefer it to stay that way. IMO, the TextMetrics class should not be aware of scrolling, it's the job of BufferView. and let the parHeight() / computeHeight() methods rely on the new vector, rather than par_metrics_[]. On the other hand par_heights_[] should definitely be a vector but I am not sure it should be in TextMetrics; BufferView is probably better. But this is only my opinion, you are doing the work so your opinion matters too. FYI I've also tried many times to rewrite from scratch this whole stuff. But at the end, incremental cleanups was much more beneficial and the result is increased stability and performance. As you've probably noticed this metrics and drawing business is very touchy and crash prone... Abdel.
Re: Scrollfix patch
> -) the boxes example now works correctly, except it is somewhat slow > because with SinglePar I'm updating and redrawing the outer Par, > not the inner one, so with such a great box (or even table, etc...), > it gets slow -- here I should add a further parameter to UpdateScope > specifying the Text where the update starts from; yes, scrolling is ok and typing is slow. moreover this patch breaks pg/up-down inside insets - which is slightly broken even now as noted in regression list. (by this i dont mean it shouldnt go in, as i suppose you'll work on the bugs further). pavel
Re: Scrollfix patch
Martin Vermeer wrote: On Sun, Nov 11, 2007 at 05:24:36AM +0100, Tommaso Cucinotta wrote: As it is somewhat growing, I wouldn't like to keep working on it for too much time further, as the operation of merging with trunk may become cumbersome. Now it seems to me sufficiently stable. T. I think this is the time to check it in -- with the trivial renames you propose. I hope you mean _without_ the trivial renames... We're not close to a trunk release so any bugs will get ironed out in a timely manner. I'd prefer that the patch is incrementally integrated. This patch should serve as a reference goal implementation. I am talking about how I would personally proceed but I am not Tommaso. It this patch goes as-is you have to promise to fixed all bugs and regressions Tommaso (I would help you of course). I'll to do a review of the patch in the coming days. Abdel.
Re: Scrollfix patch
Abdelrazak Younes wrote: It this patch goes as-is you have to promise to fixed all bugs and regressions Tommaso (I would help you of course). If this patch goes in as-is you have to promise to fix all bugs and regressions Tommaso (I would help you of course). I'll to do a review of the patch in the coming days. I'll try to do a review of the patch in the coming days. Looking at the state of my spelling, obviously not today :-) Abdel.
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 07:49:54AM +0200, Martin Vermeer wrote: > On Sun, Nov 11, 2007 at 05:24:36AM +0100, Tommaso Cucinotta wrote: > > Hi all, > > > > please, find attached an improved version of the scrollfix patch. > > > > Summary of the further changes: > > -) most optimizations for updating a single par are back > > -) actually, it is possible to specify a par range with Update::SinglePar > >-- actually, should be renamed as Update::Paragraphs; > >-- actually, Update::Force should be renamed as Update::Screen ? > > -) class UpdateScope encapsulates the scope of an update (updateFlags > >plus parameters, e.g. the par range to be updated if SinglePar is used) > > -) the boxes example now works correctly, except it is somewhat slow > >because with SinglePar I'm updating and redrawing the outer Par, > >not the inner one, so with such a great box (or even table, etc...), > >it gets slow -- here I should add a further parameter to UpdateScope > >specifying the Text where the update starts from; > > -) probably it adheres slightly more to the coding rules; > > -) updated to trunk revision 21533; > > -) needs testing to check if there are any further crashes; > > > > As it is somewhat growing, I wouldn't like to keep working on it for too > > much time further, as the operation of merging with trunk may become > > cumbersome. Now it seems to me sufficiently stable. > > > > T. > > I think this is the time to check it in -- with the trivial > renames you propose. We're not close to a trunk release so any > bugs will get ironed out in a timely manner. I'd like to see some performance measures first, lest we paint ourselves into a corner here. We had this kind of approach (all paragraph heights known) for a long time and switched to the current one for performance reasons when e.g. loading/resizing inserting in big docments. If we blindly apply fundamental changes to the paint strategy it might well be that there are some problems with the strategy that are _conceptually_ not solvable. Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 01:21:03PM +0100, Andre Poenitz wrote: > > I think this is the time to check it in -- with the trivial > > renames you propose. We're not close to a trunk release so any > > bugs will get ironed out in a timely manner. > > I'd like to see some performance measures first, lest we paint > ourselves into a corner here. An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 01:53:52PM +0100, Andre Poenitz wrote: > On Sun, Nov 11, 2007 at 01:21:03PM +0100, Andre Poenitz wrote: > > > I think this is the time to check it in -- with the trivial > > > renames you propose. We're not close to a trunk release so any > > > bugs will get ironed out in a timely manner. > > > > I'd like to see some performance measures first, lest we paint > > ourselves into a corner here. > > An entirely unscientific test (sitting in front of the computer and > counting) yields ~4s for loading the UserGuide before applying the patch > and ~13s afterwards. There is some additional debug output, but I don't > think the resulting scrolling in the terminal accounts for all of those > nine extra seconds. Looks better with the debug output disabled. Still: > So please: Produce some numbers showing that there are no inacceptable > regressions wrt to performance. A performance degradation by a factor of > 3 for loading documents does not look like a good start. Also, loading the UserGuide and keeping pressed gets stuck at some point of time. Also, on the way there the cursor is sometimes off-screen. Maybe you can also get in touch with Alfredo. He spent quite some time with the current architecture so he might be aware of more pitfalls than I am. Andre'
Re: Scrollfix patch
Andre Poenitz ha scritto: ourselves into a corner here. We had this kind of approach (all paragraph heights known) for a long time and switched to the current one for performance reasons when e.g. loading/resizing inserting in big docments. My feeling is that, even with hundreds of outer paragraphs, the process of summing their heights up should not be even perceivable on modern CPUs (provided that the heights are directly available within a vector, like it is now -- ParMetricsCache is not a map anymore, but a vector). And, more importantly, such process would be triggered only as a response to: 1) change of height of currently edited par 1.a) break of currently edited par (Enter) 2) cut and paste operations 3) width resize of screen If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a "summing (balanced) tree", that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). If you look at the patch code, you'll notice that many times (not all, though) paragraph heights are managed through dedicated methods (tm.parHeight(), tm.computeHeight(par1, par2)), instead of using the "standard" par_metrics_[p].height(), just in view of such possibility. T.
Re: Scrollfix patch
Andre Poenitz ha scritto: An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. Thanx, I'll try to figure it out... Looks better with the debug output disabled. Still: Did you get the same numbers with debug output disabled ? T.
Re: Scrollfix patch
Andre Poenitz ha scritto: An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Well, I'm verifying on my laptop. But I want to be sure to have two LyX compiled exactly with the same options, and debug disabled. On my laptop ([EMAIL PROTECTED]), it takes ~30min. a full recompilation of LyX, so I guess I'll have numbers within one hour or so (and I'm also deleting a couple of kernel source trees to accomodate for the needed space). Anyway, please note that, with the current patch, *all* par metrics and heights are pre-computed on a document load (full metrics are discarded for all pars but the visible ones), so I expect anycase a slow-down on document opening. As already mentioned on the list, I'd like to switch to an incremental computation of the document height that is made in the background while the user is already able to work on the document. I'd make such change as a further incremental patch, but if you prefer the all-in-one approach, I can go further to such step. T.
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 04:39:22PM +0100, Tommaso Cucinotta wrote: > Andre Poenitz ha scritto: >> ourselves into a corner here. We had this kind of approach (all >> paragraph heights known) for a long time and switched to the current one >> for performance reasons when e.g. loading/resizing >> inserting in big docments. >> > My feeling is that, even with hundreds of outer paragraphs, > the process of summing their heights up should not be > even perceivable on modern CPUs (provided that the heights > are directly available within a vector, like it is now -- ParMetricsCache > is not a map anymore, but a vector). And, more importantly, > such process would be triggered only as a response to: > 1) change of height of currently edited par > 1.a) break of currently edited par (Enter) > 2) cut and paste operations > 3) width resize of screen 4) Loading of a document. > If you have serious concerns about this (probably due to the > past experience on developing LyX), the best solution would > be a "summing (balanced) tree", that would exhibit O(log n) > complexity for little updates like needed in 1) [not sure about > 1.a], but probably you won't avoid the O(n) updates in case of > 2) or 3). I think loading/resizing in 4 second is ok. 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... > If you look at the patch code, you'll notice that many times > (not all, though) paragraph heights are managed through > dedicated methods (tm.parHeight(), tm.computeHeight(par1, par2)), > instead of using the "standard" par_metrics_[p].height(), just > in view of such possibility. The patch as such looks like a sensible implementation of the idea (plus/minus some minor quirks). However, as this is quite intrusive change of concept I'd rather make sure the concept is sound. And I do not really want to rely on gut feelings only here (my gut says: Id shoul be ok as we used to o it like that for a while and the lazy rebreaking was not too much of a success and machines are faster tofday then five years ago). Andre'
Re: Scrollfix patch
Andre Poenitz wrote: On Sun, Nov 11, 2007 at 04:39:22PM +0100, Tommaso Cucinotta wrote: If you have serious concerns about this (probably due to the past experience on developing LyX), the best solution would be a "summing (balanced) tree", that would exhibit O(log n) complexity for little updates like needed in 1) [not sure about 1.a], but probably you won't avoid the O(n) updates in case of 2) or 3). I think loading/resizing in 4 second is ok. I disagree with that. Loading should be as fast as possible and resizing should be instantaneous. Or we just switch to implement a WISIWIG processor. There are a lot of good things in Tommaso's patch but the initial calculation of all ParMetrics is not one of those. The TextMetrics API additions are nice but are orthogonal to our scrolling problem. 20 is rather not. I know, it's not that bad, I am exaggerating. And of course one could provide some more versose status message (as in 23/5490 paragraphs done) giving the impression that 'something' happens... That's the thing I hate most about MSWords: continuously waiting for the re-pagination to complete. We don't need a *perfect* scrolling behaviour, just a sensible one. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. Abdel.
Re: Scrollfix patch
Andre Poenitz wrote: >> 1) change of height of currently edited par >> 1.a) break of currently edited par (Enter) >> 2) cut and paste operations >> 3) width resize of screen > > 4) Loading of a document. > >> If you have serious concerns about this (probably due to the >> past experience on developing LyX), the best solution would >> be a "summing (balanced) tree", that would exhibit O(log n) >> complexity for little updates like needed in 1) [not sure about >> 1.a], but probably you won't avoid the O(n) updates in case of >> 2) or 3). > > I think loading/resizing in 4 second is ok. 20 is rather not. > I know, it's not that bad, I am exaggerating. And of course > one could provide some more versose status message (as in 23/5490 > paragraphs done) giving the impression that 'something' happens... Mmm... I think this would be pretty bad. >> If you look at the patch code, you'll notice that many times >> (not all, though) paragraph heights are managed through >> dedicated methods (tm.parHeight(), tm.computeHeight(par1, par2)), >> instead of using the "standard" par_metrics_[p].height(), just >> in view of such possibility. > > The patch as such looks like a sensible implementation of the idea > (plus/minus some minor quirks). However, as this is quite intrusive > change of concept I'd rather make sure the concept is sound. And I > do not really want to rely on gut feelings only here (my gut says: > Id shoul be ok as we used to o it like that for a while and the > lazy rebreaking was not too much of a success and machines are > faster tofday then five years ago). My main concern is that is much more difficult to go the nonlazy -> lazy route as we have done (and Andre' and myself invested a good deal of time doing it) than the other way around. So we should make this decision very carefully. PS: I *think* I agree with Andre on this point, but I rather not say it 'cause lately I'm not guessing right ;-) A/
Re: Scrollfix patch
Abdelrazak Younes wrote: >>> If you have serious concerns about this (probably due to the >>> past experience on developing LyX), the best solution would >>> be a "summing (balanced) tree", that would exhibit O(log n) >>> complexity for little updates like needed in 1) [not sure about >>> 1.a], but probably you won't avoid the O(n) updates in case of >>> 2) or 3). >> >> I think loading/resizing in 4 second is ok. > > I disagree with that. Loading should be as fast as possible and resizing > should be instantaneous. Exactly > Or we just switch to implement a WISIWIG > processor. There are a lot of good things in Tommaso's patch but the > initial calculation of all ParMetrics is not one of those. The > TextMetrics API additions are nice but are orthogonal to our scrolling > problem. > >> 20 is rather not. >> I know, it's not that bad, I am exaggerating. And of course >> one could provide some more versose status message (as in 23/5490 >> paragraphs done) giving the impression that 'something' happens... > > That's the thing I hate most about MSWords: continuously waiting for the > re-pagination to complete. We don't need a *perfect* scrolling > behaviour, just a sensible one. A simple cache of paragraph height would > do the job and I wish Tommaso had followed this direction instead. I agree 100%. A/
Re: Scrollfix patch
Andre Poenitz wrote: > On Sun, Nov 11, 2007 at 01:21:03PM +0100, Andre Poenitz wrote: >> > I think this is the time to check it in -- with the trivial >> > renames you propose. We're not close to a trunk release so any >> > bugs will get ironed out in a timely manner. >> >> I'd like to see some performance measures first, lest we paint >> ourselves into a corner here. > > An entirely unscientific test (sitting in front of the computer and > counting) yields ~4s for loading the UserGuide before applying the patch > and ~13s afterwards. There is some additional debug output, but I don't > think the resulting scrolling in the terminal accounts for all of those > nine extra seconds. > > So please: Produce some numbers showing that there are no inacceptable > regressions wrt to performance. A performance degradation by a factor of > 3 for loading documents does not look like a good start. A small point: when testing we should not limit ourselves to the Userguide. IMO one of the really strong points of LyX is to be able to deal with huge documents, like books. (the UG has just 150 pages, it is like a small book). Btw, we used to test performance with UG20 (just 20 copies of UserGuide) some time (but I agree that this is kind of extreme, though). A/
Re: Scrollfix patch
Tommaso Cucinotta wrote: > Andre Poenitz ha scritto: >> ourselves into a corner here. We had this kind of approach (all >> paragraph heights known) for a long time and switched to the >> current one for performance reasons when e.g. loading/resizing >> inserting in big docments. >> > My feeling is that, even with hundreds of outer paragraphs, > the process of summing their heights up should not be > even perceivable on modern CPUs (provided that the heights > are directly available within a vector, like it is now -- ParMetricsCache > is not a map anymore, but a vector). And, more importantly, > such process would be triggered only as a response to: > 1) change of height of currently edited par >1.a) break of currently edited par (Enter) > 2) cut and paste operations > 3) width resize of screen 3) document loading 4) graphics updating -> this is particular important with math previews on a document full of them. > If you have serious concerns about this (probably due to the > past experience on developing LyX), the best solution would > be a "summing (balanced) tree", that would exhibit O(log n) > complexity for little updates like needed in 1) [not sure about > 1.a], but probably you won't avoid the O(n) updates in case of > 2) or 3). This could be a good idea (if needed of course, numbers should tell). A/
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 05:25:20PM +0100, Tommaso Cucinotta wrote: > Andre Poenitz ha scritto: >> An entirely unscientific test (sitting in front of the computer and >> counting) yields ~4s for loading the UserGuide before applying the patch >> and ~13s afterwards. There is some additional debug output, but I don't >> think the resulting scrolling in the terminal accounts for all of those >> nine extra seconds. >> >> So please: Produce some numbers showing that there are no inacceptable >> regressions wrt to performance. A performance degradation by a factor of >> 3 for loading documents does not look like a good start. >> > Well, I'm verifying on my laptop. But I want to be sure to have > two LyX compiled exactly with the same options, and debug > disabled. On my laptop ([EMAIL PROTECTED]), it takes ~30min. a full > recompilation of LyX, so I guess I'll have numbers within one > hour or so (and I'm also deleting a couple of kernel source > trees to accomodate for the needed space). > > Anyway, please note that, with the current patch, *all* par > metrics and heights are pre-computed on a document load > (full metrics are discarded for all pars but the visible ones), > so I expect anycase a slow-down on document opening. Some performance decrease for loading will be acceptable... > As already mentioned on the list, I'd like to switch to an > incremental computation of the document height that is > made in the background while the user is already able to > work on the document. > I'd make such change as a further incremental patch, but if > you prefer the all-in-one approach, I can go further to such > step. "Incremental" is fine if the direction is ok and each step leaves us with a usable code base. Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 06:21:45PM +0100, Abdelrazak Younes wrote: >> 20 is rather not. >> I know, it's not that bad, I am exaggerating. And of course >> one could provide some more versose status message (as in 23/5490 >> paragraphs done) giving the impression that 'something' happens... > > That's the thing I hate most about MSWords: continuously waiting for the > re-pagination to complete. We don't need a *perfect* scrolling behaviour, > just a sensible one. A simple cache of paragraph height would do the job > and I wish Tommaso had followed this direction instead. So looks like Tommasso and you will need to talk to each other ;-) Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 06:46:38PM +0100, Alfredo Braunstein wrote: > Andre Poenitz wrote: > > > On Sun, Nov 11, 2007 at 01:21:03PM +0100, Andre Poenitz wrote: > >> > I think this is the time to check it in -- with the trivial > >> > renames you propose. We're not close to a trunk release so any > >> > bugs will get ironed out in a timely manner. > >> > >> I'd like to see some performance measures first, lest we paint > >> ourselves into a corner here. > > > > An entirely unscientific test (sitting in front of the computer and > > counting) yields ~4s for loading the UserGuide before applying the patch > > and ~13s afterwards. There is some additional debug output, but I don't > > think the resulting scrolling in the terminal accounts for all of those > > nine extra seconds. > > > > So please: Produce some numbers showing that there are no inacceptable > > regressions wrt to performance. A performance degradation by a factor of > > 3 for loading documents does not look like a good start. > > A small point: when testing we should not limit ourselves to the Userguide. > IMO one of the really strong points of LyX is to be able to deal with huge > documents, like books. (the UG has just 150 pages, it is like a small > book). Btw, we used to test performance with UG20 (just 20 copies of > UserGuide) some time (but I agree that this is kind of extreme, though). But if a concept does not scale up well it's usually a bad concept. So checking with ug20 is certainly still a vaild approach. Andre'
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 08:08:52PM +0100, Andre Poenitz wrote: > On Sun, Nov 11, 2007 at 05:25:20PM +0100, Tommaso Cucinotta wrote: > > Andre Poenitz ha scritto: > >> An entirely unscientific test (sitting in front of the computer and > >> counting) yields ~4s for loading the UserGuide before applying the patch > >> and ~13s afterwards. There is some additional debug output, but I don't > >> think the resulting scrolling in the terminal accounts for all of those > >> nine extra seconds. > >> > >> So please: Produce some numbers showing that there are no inacceptable > >> regressions wrt to performance. A performance degradation by a factor of > >> 3 for loading documents does not look like a good start. > >> > > Well, I'm verifying on my laptop. But I want to be sure to have > > two LyX compiled exactly with the same options, and debug > > disabled. On my laptop ([EMAIL PROTECTED]), it takes ~30min. a full > > recompilation of LyX, so I guess I'll have numbers within one > > hour or so (and I'm also deleting a couple of kernel source > > trees to accomodate for the needed space). > > > > Anyway, please note that, with the current patch, *all* par > > metrics and heights are pre-computed on a document load > > (full metrics are discarded for all pars but the visible ones), > > so I expect anycase a slow-down on document opening. > > Some performance decrease for loading will be acceptable... But it looks like Abdel and Alfredo disagree. So maybe some kind of discussion is needed. Andre'
Re: Scrollfix patch
Andre Poenitz wrote: On Sun, Nov 11, 2007 at 06:46:38PM +0100, Alfredo Braunstein wrote: A small point: when testing we should not limit ourselves to the Userguide. IMO one of the really strong points of LyX is to be able to deal with huge documents, like books. (the UG has just 150 pages, it is like a small book). Btw, we used to test performance with UG20 (just 20 copies of UserGuide) some time (but I agree that this is kind of extreme, though). Yes, this plus the fact that one often loads many documents at startup. But if a concept does not scale up well it's usually a bad concept. So checking with ug20 is certainly still a vaild approach. And it is always my approach when testing file loading or label/toc updates. Abdel.
Re: Scrollfix patch
Ok, I have my own numbers. This is what I obtained launching LyX from the shell asking to open a document, and hitting Alt-F4 (close application) as soon as I see the LyX window. Note that, before LyX is closed, I can see clearly the loaded document showing on the screen, then the pressed sequence closes LyX (therefore in the patched case, full par metrics and heights computation is done). Note also that I didn't report numbers from the *first* execution, but I repeated the process 3 or 4 times, then I reported the last numbers. This is done to reach a "fair" condition in which all files are basically cached in the Linux disk cache when launching the program. This is trunk, as checked out ~2 hour ago (USERGUIDE.LYX): [EMAIL PROTECTED]:~/lyx-trunk$ time sudo nice ./src/lyx lib/doc/UserGuide.lyx > /dev/null 2>&1 real0m3.057s user0m2.127s sys 0m0.112s This is my scrollfix patch, as sent to the list tonight: [EMAIL PROTECTED]:~/lyx-devel$ time sudo nice ./src/lyx lib/doc/UserGuide.lyx > /dev/null 2>&1 real0m2.611s user0m1.719s sys 0m0.110s And now again trunk for EMBEDDEDOBJECTS.LYX: [EMAIL PROTECTED]:~/lyx-trunk$ time sudo nice ./src/lyx lib/doc/EmbeddedObjects.lyx > /dev/null 2>&1 real0m2.887s user0m1.912s sys 0m0.141s and again my scrollfix patch: [EMAIL PROTECTED]:~/lyx-devel$ time sudo nice ./src/lyx lib/doc/EmbeddedObjects.lyx > /dev/null 2>&1 real0m3.156s user0m2.269s sys 0m0.106s Conclusions: the unscientific experiment of Andre Poenits was really non-scientific. I guess he must have left debug enabled when trying the scrollfix, and disabled in the other case :-) Another possibility could be he tried the scrollfix with an empty disk cache, then trunk with a filled disk-cache. The difference is enormous, as shown below: This is what happens to trunk if I clear the disk cache: [EMAIL PROTECTED]:~# sync && echo 3 > /proc/sys/vm/drop_caches [EMAIL PROTECTED]:~/lyx-trunk$ time sudo nice ./src/lyx lib/doc/EmbeddedObjects.lyx > /dev/null 2>&1 real0m18.461s user0m1.952s sys 0m0.195s and this is my scrollfix after the same process: [EMAIL PROTECTED]:~# sync && echo 3 > /proc/sys/vm/drop_caches [EMAIL PROTECTED]:~/lyx-devel$ time sudo nice ./src/lyx lib/doc/EmbeddedObjects.lyx > /dev/null 2>&1 real0m19.146s user0m2.268s sys 0m0.187s As you can see, ~20 seconds are needed to load LyX along with all the shared libraries it uses, configuration files, and the document itself. The time actually spent by the CPU does not change that much (~2 sec.). If anyone has a longer file (a Master thesis, PhD thesis or something like that), I can try to see what is the doc length at which things start really slowing down inaccpetably. T. Tommaso Cucinotta ha scritto: Andre Poenitz ha scritto: An entirely unscientific test (sitting in front of the computer and counting) yields ~4s for loading the UserGuide before applying the patch and ~13s afterwards. There is some additional debug output, but I don't think the resulting scrolling in the terminal accounts for all of those nine extra seconds. So please: Produce some numbers showing that there are no inacceptable regressions wrt to performance. A performance degradation by a factor of 3 for loading documents does not look like a good start. Well, I'm verifying on my laptop. But I want to be sure to have two LyX compiled exactly with the same options, and debug disabled. On my laptop ([EMAIL PROTECTED]), it takes ~30min. a full recompilation of LyX, so I guess I'll have numbers within one hour or so (and I'm also deleting a couple of kernel source trees to accomodate for the needed space). Anyway, please note that, with the current patch, *all* par metrics and heights are pre-computed on a document load (full metrics are discarded for all pars but the visible ones), so I expect anycase a slow-down on document opening. As already mentioned on the list, I'd like to switch to an incremental computation of the document height that is made in the background while the user is already able to work on the document. I'd make such change as a further incremental patch, but if you prefer the all-in-one approach, I can go further to such step. T. -- Tommaso Cucinotta, Computer Engineering PhD, Researcher ReTiS Lab, Scuola Superiore Sant'Anna, Pisa, Italy Tel +39 050 882 024, Fax +39 050 882 003 http://feanor.sssup.it/~tommaso
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 08:59:01PM +0100, Tommaso Cucinotta wrote: > Conclusions: the unscientific experiment of Andre Poenits was really > non-scientific. I guess he must have left debug enabled when trying > the scrollfix, and disabled in the other case :-) Another possibility > could be he tried the scrollfix with an empty disk cache, then trunk > with a filled disk-cache. The difference is enormous, as shown below: Note that your patch created lots of output by itself. So when I test trunk vs patch I "magically" get more debug output. > If anyone has a longer file (a Master thesis, PhD thesis or something > like that), I can try to see what is the doc length at which things > start really slowing down inaccpetably. Just copy the UserGuide 20 times into itself. Andre'
Re: Scrollfix patch
Abdelrazak Younes ha scritto: We don't need a *perfect* scrolling behaviour, just a sensible one. I agree we may accept a non-perfect scrolling right after having opened a document, or having made great changes. But, IMHO, after a while it would be better to have a precise behaviour, while reading you doc, searching for places to improve/change and editing. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. Well, this is actually what I did, except I'm caching also the width, the ascent and the descent, along with the height. Thought they could have turned out to be useful, someday. Basically, first I added a vector of heights, but immediately after I realized it was sufficient to reuse the ParagraphMetrics type, with the rows_ field cleared. So, the par_metrics_[] vector contains width, height, asc and desc of all *outer* paragraphs in the document, while only visible ones have the .rows() collection filled in. Well, it is not really a cache in the "cache" sense... T.
Re: Scrollfix patch
Andre Poenitz ha scritto: Just copy the UserGuide 20 times into itself. Done. I get a time roughly doubled: trunk: 5.5 secs scrollfix: 10.5 secs Well, I guess nobody really works with so large documents, but you have at least one file per chapter. Though, I can add the background-incremental height computation behaviour to my patch, in an attempt to make everybody happy. T.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Abdelrazak Younes ha scritto: We don't need a *perfect* scrolling behaviour, just a sensible one. I agree we may accept a non-perfect scrolling right after having opened a document, or having made great changes. But, IMHO, after a while it would be better to have a precise behaviour, while reading you doc, searching for places to improve/change and editing. I think that updating the par height the moment it is displayed on screen would be enough. A simple cache of paragraph height would do the job and I wish Tommaso had followed this direction instead. Well, this is actually what I did, except I'm caching also the width, the ascent and the descent, along with the height. Thought they could have turned out to be useful, someday. I don't think this is useful. In any case, it is not useful now so why the memory waste? Basically, first I added a vector of heights, but immediately after I realized it was sufficient to reuse the ParagraphMetrics type, with the rows_ field cleared. I understand your reasoning but I disagree, sorry... see below. So, the par_metrics_[] vector contains width, height, asc and desc of all *outer* paragraphs in the document, while only visible ones have the .rows() collection filled in. I don't like this distinction. I prefer all or nothing. Actually I planned to have partial metrics also for non-main Insets. You are doing the other way around: full metrics for the main Inset. Well, it is not really a cache in the "cache" sense... Not in your patch no. Abdel.
Re: Scrollfix patch
Abdelrazak Younes ha scritto: I don't like this distinction. I prefer all or nothing. Well, if you really think it is preferreable, then it would probably be a matter of a few minutes to create a new vector TextMetrics::par_heights_[], in addition to the par_metrics_[] one, and let the parHeight() / computeHeight() methods rely on the new vector, rather than par_metrics_[]. T.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Abdelrazak Younes ha scritto: I don't like this distinction. I prefer all or nothing. Well, if you really think it is preferreable, then it would probably be a matter of a few minutes to create a new vector TextMetrics::par_heights_[], in addition to the par_metrics_[] one, Well, I don't think par_metrics_ should become a vector... I used a map to minimize memory and simplify access (no worry about resize etc). The inset metrics, the CoordCache and the Rows all behave in a consistent way. I fixed an unbelievable number of bugs thanks to this consistency. This is a simple mental model that works well now and I'd prefer it to stay that way. IMO, the TextMetrics class should not be aware of scrolling, it's the job of BufferView. > and let the parHeight() / computeHeight() methods rely on the new vector, rather than par_metrics_[]. On the other hand par_heights_[] should definitely be a vector but I am not sure it should be in TextMetrics; BufferView is probably better. But this is only my opinion, you are doing the work so your opinion matters too. FYI I've also tried many times to rewrite from scratch this whole stuff. But at the end, incremental cleanups was much more beneficial and the result is increased stability and performance. As you've probably noticed this metrics and drawing business is very touchy and crash prone... Abdel.
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 05:24:36AM +0100, Tommaso Cucinotta wrote: Hi all, please, find attached an improved version of the scrollfix patch. Summary of the further changes: -) most optimizations for updating a single par are back -) actually, it is possible to specify a par range with Update::SinglePar -- actually, should be renamed as Update::Paragraphs; -- actually, Update::Force should be renamed as Update::Screen ? -) class UpdateScope encapsulates the scope of an update (updateFlags plus parameters, e.g. the par range to be updated if SinglePar is used) -) the boxes example now works correctly, except it is somewhat slow because with SinglePar I'm updating and redrawing the outer Par, not the inner one, so with such a great box (or even table, etc...), it gets slow -- here I should add a further parameter to UpdateScope specifying the Text where the update starts from; -) probably it adheres slightly more to the coding rules; -) updated to trunk revision 21533; -) needs testing to check if there are any further crashes; As it is somewhat growing, I wouldn't like to keep working on it for too much time further, as the operation of merging with trunk may become cumbersome. Now it seems to me sufficiently stable. T. I think this is the time to check it in -- with the trivial renames you propose. We're not close to a trunk release so any bugs will get ironed out in a timely manner. - Martin
Re: Scrollfix patch
On Sun, Nov 11, 2007 at 05:24:36AM +0100, Tommaso Cucinotta wrote: > Hi all, > > please, find attached an improved version of the scrollfix patch. > > Summary of the further changes: > -) most optimizations for updating a single par are back > -) actually, it is possible to specify a par range with Update::SinglePar >-- actually, should be renamed as Update::Paragraphs; >-- actually, Update::Force should be renamed as Update::Screen ? > -) class UpdateScope encapsulates the scope of an update (updateFlags >plus parameters, e.g. the par range to be updated if SinglePar is used) > -) the boxes example now works correctly, except it is somewhat slow >because with SinglePar I'm updating and redrawing the outer Par, >not the inner one, so with such a great box (or even table, etc...), >it gets slow -- here I should add a further parameter to UpdateScope >specifying the Text where the update starts from; > -) probably it adheres slightly more to the coding rules; > -) updated to trunk revision 21533; > -) needs testing to check if there are any further crashes; > > As it is somewhat growing, I wouldn't like to keep working on it for too > much time further, as the operation of merging with trunk may become > cumbersome. Now it seems to me sufficiently stable. > > T. I think this is the time to check it in -- with the trivial renames you propose. We're not close to a trunk release so any bugs will get ironed out in a timely manner. - Martin
Re: Scrollfix patch
Tommaso Cucinotta wrote: Helge Hafting ha scritto: 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. Please, note that the updateFlags are still correctly produced by the various (quite obscure) code segments around. Except that they are ignored at the final rendering time. Actually, I didn't like the fact that, in the current trunk, AFAICU, the ViewMetricsInfo object is used, in the BufferView, as a kind of reminder of what needs to be refreshed. Then, in response to the same request (should be buffer_.changed()), the textMetrics.draw() method either draws the entire screen or just a paragraph depending on such reminder. My next step would just be to rely on different signals, in the framework: -) one for updating the entire screen as a result of view-specific events, such as changing the anchor (i.e. scrolling), the zoom factor, what you see or hide, Ideally, you don't update the entire screen on scrolling. If the scroll is less than a screenfull, just let the windowing system move the part that stays visible. And then you draw only the exposed part. A nice speedup, especially when working across the network. etc... -- this process should theoretically not interest the buffer anyway -) one for updating a single paragraph as a result of changing its contents (e.g. when you type characters) -- this should also trigger a redraw from the paragraph to the bottom if the height changes, Ideally, paint only the single line containing the cursor, when the rest of the paragraph stays unchanged. More important: Don't do a redraw to the bottom, just move the stuff down and redraw the paragraph only. So correct scrolling beats fast but erratic scrolling. Still, I hope the optimizations comes back after a while. :-) Yes, this is the plan. Good to hear! Helge Hafting
Re: Scrollfix patch
On Thu, 08 Nov 2007 15:33:55 +0100 Tommaso Cucinotta [EMAIL PROTECTED] wrote: ... It seems in this practical world, nobody cares anymore about optimizations. After all, we have to give something to do to our quad-core machines, right ? :-) I know perfectly what you're talking about, so, if we can manage to have something stable and correctly behaving, then we can even go through such optimizations. Probably, the LyX UpdateFlags will need a few further enum values, allowing all of such possibilities. I think _in practice_ it is enough to have row refresh and all-screen refresh (and no refresh at all, when only moving the cursor). When typing, all-screen refresh should happen only rarely, when the current row is re-broken. This is where the slowness is painful to the user if we refresh the whole screen for every single keystroke. - Martin
Re: Scrollfix patch
Helge Hafting ha scritto: Ideally, you don't update the entire screen on scrolling. If the scroll is less than a screenfull, just let the windowing system move the part that stays visible. And then you draw only the exposed part. A nice speedup, especially when working across the network. Ideally :-) Ideally, paint only the single line containing the cursor, when the rest of the paragraph stays unchanged. More important: Don't do a redraw to the bottom, just move the stuff down and redraw the paragraph only. Ideally :-) Ideally, don't even redraw the entire line, but just the new entered character, unless it triggered a respacing of the entire line due to justification. Ideally :-) It seems in this practical world, nobody cares anymore about optimizations. After all, we have to give something to do to our quad-core machines, right ? :-) I know perfectly what you're talking about, so, if we can manage to have something stable and correctly behaving, then we can even go through such optimizations. Probably, the LyX UpdateFlags will need a few further enum values, allowing all of such possibilities. T.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Helge Hafting ha scritto: 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. Please, note that the updateFlags are still correctly produced by the various (quite obscure) code segments around. Except that they are ignored at the final rendering time. Actually, I didn't like the fact that, in the current trunk, AFAICU, the ViewMetricsInfo object is used, in the BufferView, as a kind of "reminder" of what needs to be refreshed. Then, in response to the same "request" (should be buffer_.changed()), the textMetrics.draw() method either draws the entire screen or just a paragraph depending on such "reminder". My next step would just be to rely on different signals, in the framework: -) one for updating the entire screen as a result of view-specific events, such as changing the anchor (i.e. scrolling), the zoom factor, what you see or hide, Ideally, you don't update the entire screen on scrolling. If the scroll is less than a screenfull, just let the windowing system move the part that stays visible. And then you draw only the exposed part. A nice speedup, especially when working across the network. etc... -- this process should theoretically not interest the buffer anyway -) one for updating a single paragraph as a result of changing its contents (e.g. when you type characters) -- this should also trigger a redraw from the paragraph to the bottom if the height changes, Ideally, paint only the single line containing the cursor, when the rest of the paragraph stays unchanged. More important: Don't do a redraw to the bottom, just move the stuff down and redraw the paragraph only. So correct scrolling beats fast but erratic scrolling. Still, I hope the optimizations comes back after a while. :-) Yes, this is the plan. Good to hear! Helge Hafting
Re: Scrollfix patch
On Thu, 08 Nov 2007 15:33:55 +0100 Tommaso Cucinotta <[EMAIL PROTECTED]> wrote: ... > It seems in this practical world, nobody cares anymore about optimizations. > After all, we have to give something to do to our quad-core machines, > right ? :-) > > I know perfectly what you're talking about, so, if we can manage > to have something stable and correctly behaving, then we can even go through > such optimizations. Probably, the LyX UpdateFlags will need a few > further enum > values, allowing all of such possibilities. I think _in practice_ it is enough to have row refresh and all-screen refresh (and no refresh at all, when only moving the cursor). When typing, all-screen refresh should happen only rarely, when the current row is re-broken. This is where the slowness is painful to the user if we refresh the whole screen for every single keystroke. - Martin
Re: Scrollfix patch
Helge Hafting ha scritto: Ideally, you don't update the entire screen on scrolling. If the scroll is less than a screenfull, just let the windowing system move the part that stays visible. And then you draw only the exposed part. A nice speedup, especially when working across the network. Ideally :-) Ideally, paint only the single line containing the cursor, when the rest of the paragraph stays unchanged. More important: Don't do a redraw to the bottom, just move the stuff down and redraw the paragraph only. Ideally :-) Ideally, don't even redraw the entire line, but just the new entered character, unless it triggered a respacing of the entire line due to justification. Ideally :-) It seems in this practical world, nobody cares anymore about optimizations. After all, we have to give something to do to our quad-core machines, right ? :-) I know perfectly what you're talking about, so, if we can manage to have something stable and correctly behaving, then we can even go through such optimizations. Probably, the LyX UpdateFlags will need a few further enum values, allowing all of such possibilities. T.
Re: Scrollfix patch
Tommaso Cucinotta wrote: Hi Abdel, please find attached a first attempt of fixing the non-uniform scrolling of LyX on trunk (21411). Very good Tommaso! I know this represents a lot of work but there's still some things to improve in your patch. I hope you won't let us down :-). The downside of your patch is that it erases most of the optimisation I've worked hard to preserve. The upside of it is of course that you obviously gained a lot of knowledge in the process and I am very happy about that. My general comments in no particular order: 1) the patch is not in sync with trunk 2) when you comment out code because it is not needed anymore, just delete it instead. 3) when you comment out code because you don't know (yet) how to upgrade it with the new architecture, then put a FIXME explaining the issue and the potential solution. 4) we are a bunch of code rules fundamentalists so please read the docs in trunk/development/coding/ and update your patch in consequence. 5) there are a certain number of cleanups that can be committed sequentially: the TextAnchor part, the TexMetrics part, etc. I believe we can integrate your patch step by step. 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. Having said that, I like very much the direction of your patch :-) I suggest that you 'try' to gain commit privilege and that you integrate your patch step by step by first posting the patch to the list so that I and others can comment easily. I say 'try' because I am not sure you would get it but please try anyway because I really don't feel like doing it for you. If someone else steps up that's fine too. Abdel.
Re: Scrollfix patch
Abdelrazak Younes wrote: Tommaso Cucinotta wrote: Hi Abdel, please find attached a first attempt of fixing the non-uniform scrolling of LyX on trunk (21411). 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. I have had much trouble with performance. Still, I think that correctness is more important than performance. And the scrolling anomalies have been bad in some cases, especially short/medium sized documents containing big objects like full-page images. So correct scrolling beats fast but erratic scrolling. Still, I hope the optimizations comes back after a while. :-) Helge Hafting
Re: Scrollfix patch
please find attached a first attempt of fixing the non-uniform scrolling of LyX on trunk (21411). it seems this patch breaks view of boxes. try e.g. http://bugzilla.lyx.org/attachment.cgi?id=2218action=view in 1.5 and in patched trunk to see the difference. paragraph does not respect width of box and boxes are not fixed in width. anyway, i'm happy somebody started fixing this very annoying bug. pavel
Re: Scrollfix patch
Abdelrazak Younes ha scritto: hope you won't let us down :-). The downside of your patch is that it erases most of the optimisation I've worked hard to preserve. Guess I can preserve such optimizations (i.e. the updateFlags computation and management) -- see also my previous msg. My aim is to reintroduce the use of such flags (that are computed by the core text editing or cursor movement features) in order to either trigger refresh of the entire screen, or of a single par (possibly along with the subsequent pars, if needed). Concerning coding guidelines and your general comments below, I'll take them into account once I come up with a complete patch. Probably I'm not completely sure I'll be able to split easily the patch into multiple pieces, let's see what comes out, then we'll discuss on how to integrate changes. On a related note, is anyone currently working on the implementation of a horizontal scrollbar ? Bye, T. The upside of it is of course that you obviously gained a lot of knowledge in the process and I am very happy about that. My general comments in no particular order: 1) the patch is not in sync with trunk 2) when you comment out code because it is not needed anymore, just delete it instead. 3) when you comment out code because you don't know (yet) how to upgrade it with the new architecture, then put a FIXME explaining the issue and the potential solution. 4) we are a bunch of code rules fundamentalists so please read the docs in trunk/development/coding/ and update your patch in consequence. 5) there are a certain number of cleanups that can be committed sequentially: the TextAnchor part, the TexMetrics part, etc. I believe we can integrate your patch step by step. 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. Having said that, I like very much the direction of your patch :-) I suggest that you 'try' to gain commit privilege and that you integrate your patch step by step by first posting the patch to the list so that I and others can comment easily. I say 'try' because I am not sure you would get it but please try anyway because I really don't feel like doing it for you. If someone else steps up that's fine too. Abdel.
Re: Scrollfix patch
Helge Hafting ha scritto: 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. Please, note that the updateFlags are still correctly produced by the various (quite obscure) code segments around. Except that they are ignored at the final rendering time. Actually, I didn't like the fact that, in the current trunk, AFAICU, the ViewMetricsInfo object is used, in the BufferView, as a kind of reminder of what needs to be refreshed. Then, in response to the same request (should be buffer_.changed()), the textMetrics.draw() method either draws the entire screen or just a paragraph depending on such reminder. My next step would just be to rely on different signals, in the framework: -) one for updating the entire screen as a result of view-specific events, such as changing the anchor (i.e. scrolling), the zoom factor, what you see or hide, etc... -- this process should theoretically not interest the buffer anyway -) one for updating a single paragraph as a result of changing its contents (e.g. when you type characters) -- this should also trigger a redraw from the paragraph to the bottom if the height changes, of if neighbour paragraphs are affected as well (e.g. hitting enter to split a par into two) -- this process should probably start from the buffer itself, that should trigger a paragraph refresh of (all) the view(s) showing that buffer. This would eliminate the slowness (re)introduced by the current patch. I have had much trouble with performance. Still, I think that correctness is more important than performance. And the scrolling anomalies have been bad in some cases, especially short/medium sized documents containing big objects like full-page images. So correct scrolling beats fast but erratic scrolling. Still, I hope the optimizations comes back after a while. :-) Yes, this is the plan. T.
Re: Scrollfix patch
On Wed, Nov 07, 2007 at 12:04:46AM +0100, Tommaso Cucinotta wrote:
Any comments by anyone is welcome.
It does not apply to trunk:
patching file src/ParagraphMetrics.h
patching file src/insets/InsetInclude.cpp
patching file src/insets/InsetText.cpp
patching file src/BufferView.cpp
Hunk #1 FAILED at 349.
Hunk #2 FAILED at 369.
Hunk #3 succeeded at 329 (offset -130 lines).
Hunk #4 succeeded at 385 (offset -141 lines).
Hunk #5 succeeded at 405 (offset -141 lines).
Hunk #6 succeeded at 443 (offset -142 lines).
Hunk #7 FAILED at 472.
Hunk #8 succeeded at 530 (offset -142 lines).
Hunk #9 succeeded at 640 (offset -142 lines).
Hunk #10 succeeded at 1123 with fuzz 1 (offset -166 lines).
Hunk #11 succeeded at 1135 (offset -164 lines).
Hunk #12 succeeded at 1289 (offset -162 lines).
Hunk #13 succeeded at 1324 (offset -162 lines).
Hunk #14 FAILED at 1336.
Hunk #15 succeeded at 1393 (offset -162 lines).
Hunk #16 succeeded at 1532 (offset -162 lines).
Hunk #17 FAILED at 1568.
Hunk #18 succeeded at 1579 (offset -166 lines).
Hunk #19 succeeded at 1600 (offset -166 lines).
Hunk #20 succeeded at 1612 (offset -166 lines).
Hunk #21 succeeded at 1635 (offset -166 lines).
Hunk #22 succeeded at 1651 (offset -166 lines).
Hunk #23 succeeded at 1785 (offset -166 lines).
Hunk #24 succeeded at 1827 (offset -166 lines).
Hunk #25 succeeded at 1836 (offset -166 lines).
Hunk #26 FAILED at 1848.
Hunk #27 succeeded at 2035 (offset -150 lines).
6 out of 27 hunks FAILED -- saving rejects to file src/BufferView.cpp.rej
patching file src/TextMetrics.h
patching file src/TextMetrics.cpp
Hunk #1 succeeded at 134 (offset 1 line).
Hunk #2 succeeded at 392 (offset 1 line).
Hunk #3 succeeded at 420 with fuzz 1 (offset 1 line).
Hunk #4 succeeded at 500 (offset 1 line).
Hunk #5 succeeded at 530 (offset 1 line).
Hunk #6 succeeded at 564 (offset 1 line).
Hunk #7 succeeded at 921 (offset 1 line).
Hunk #8 succeeded at 1124 (offset 1 line).
Hunk #9 succeeded at 1269 (offset 1 line).
Hunk #10 succeeded at 1309 (offset 1 line).
Hunk #11 succeeded at 1378 (offset 1 line).
Hunk #12 succeeded at 1405 (offset 1 line).
Hunk #13 succeeded at 1440 (offset 1 line).
Hunk #14 succeeded at 1464 (offset 1 line).
Hunk #15 succeeded at 1498 (offset 1 line).
Hunk #16 succeeded at 1543 (offset 1 line).
Hunk #17 succeeded at 1765 (offset 1 line).
Hunk #18 succeeded at 2010 (offset 1 line).
Hunk #19 succeeded at 2019 (offset 1 line).
Hunk #20 succeeded at 2133 (offset 3 lines).
Hunk #21 succeeded at 2278 (offset 3 lines).
patching file src/ParagraphMetrics.cpp
patching file src/frontends/WorkArea.h
patching file src/frontends/qt4/GuiView.cpp
patching file src/frontends/WorkArea.cpp
patching file src/Cursor.cpp
Hunk #1 succeeded at 1224 (offset 2 lines).
Hunk #2 succeeded at 1272 (offset 2 lines).
patching file src/BufferView.h
Hunk #6 succeeded at 211 with fuzz 2.
Hunk #7 succeeded at 219 (offset -1 lines).
Hunk #8 succeeded at 267 (offset -1 lines).
Hunk #9 succeeded at 302 (offset -3 lines).
patching file src/Text.h
Index: src/ParagraphMetrics.h
===
--- src/ParagraphMetrics.h(revisione 21411)
+++ src/ParagraphMetrics.h(copia locale)
@@ -113,6 +113,7 @@
InsetDims inset_dims_;
};
+
} // namespace lyx
Cosmetic changes please outside of significant changes like this one.
+bool BufferView::fitCursor() {
+ return fitCursor(d-cursor_);
+}
Brace on new line for function definitions.
@@ -1342,7 +1289,8 @@
case LFUN_SCREEN_UP:
case LFUN_SCREEN_DOWN: {
Point p = getPos(cur, cur.boundary());
- if (p.y_ 0 || p.y_ height_) {
+ if (cursorStatus(cur) != CUR_INSIDE)
+ {
But not inside.
Index: src/BufferView.h
===
--- src/BufferView.h (revisione 21411)
+++ src/BufferView.h (copia locale)
@@ -19,6 +19,7 @@
#include support/strfwd.h
#include support/types.h
+#include boost/signal.hpp
Ok as long as the patch is developed, but I'd like something else in the
end.
Do you have any timing results (like loading the UserGuide before/after,
inserting a paragraph at the bin/middle/end of document)?
Andre'
Re: Scrollfix patch
Tommaso Cucinotta wrote: Hi Abdel, please find attached a first attempt of fixing the non-uniform scrolling of LyX on trunk (21411). Very good Tommaso! I know this represents a lot of work but there's still some things to improve in your patch. I hope you won't let us down :-). The downside of your patch is that it erases most of the optimisation I've worked hard to preserve. The upside of it is of course that you obviously gained a lot of knowledge in the process and I am very happy about that. My general comments in no particular order: 1) the patch is not in sync with trunk 2) when you comment out code because it is not needed anymore, just delete it instead. 3) when you comment out code because you don't know (yet) how to upgrade it with the new architecture, then put a FIXME explaining the issue and the potential solution. 4) we are a bunch of code rules fundamentalists so please read the docs in "trunk/development/coding/" and update your patch in consequence. 5) there are a certain number of cleanups that can be committed sequentially: the TextAnchor part, the TexMetrics part, etc. I believe we can integrate your patch step by step. 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. Having said that, I like very much the direction of your patch :-) I suggest that you 'try' to gain commit privilege and that you integrate your patch step by step by first posting the patch to the list so that I and others can comment easily. I say 'try' because I am not sure you would get it but please try anyway because I really don't feel like doing it for you. If someone else steps up that's fine too. Abdel.
Re: Scrollfix patch
Abdelrazak Younes wrote: Tommaso Cucinotta wrote: Hi Abdel, please find attached a first attempt of fixing the non-uniform scrolling of LyX on trunk (21411). 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. I have had much trouble with performance. Still, I think that correctness is more important than performance. And the scrolling anomalies have been bad in some cases, especially short/medium sized documents containing big objects like full-page images. So correct scrolling beats fast but erratic scrolling. Still, I hope the optimizations comes back after a while. :-) Helge Hafting
Re: Scrollfix patch
> please find attached a first attempt of fixing the non-uniform scrolling > of LyX on trunk (21411). it seems this patch breaks view of boxes. try e.g. http://bugzilla.lyx.org/attachment.cgi?id=2218=view in 1.5 and in patched trunk to see the difference. paragraph does not respect width of box and boxes are not fixed in width. anyway, i'm happy somebody started fixing this very annoying bug. pavel
Re: Scrollfix patch
Abdelrazak Younes ha scritto: hope you won't let us down :-). The downside of your patch is that it erases most of the optimisation I've worked hard to preserve. Guess I can preserve such optimizations (i.e. the updateFlags computation and management) -- see also my previous msg. My aim is to reintroduce the use of such flags (that are computed by the core text editing or cursor movement features) in order to either trigger refresh of the entire screen, or of a single par (possibly along with the subsequent pars, if needed). Concerning coding guidelines and your general comments below, I'll take them into account once I come up with a complete patch. Probably I'm not completely sure I'll be able to split easily the patch into multiple pieces, let's see what comes out, then we'll discuss on how to integrate changes. On a related note, is anyone currently working on the implementation of a horizontal scrollbar ? Bye, T. The upside of it is of course that you obviously gained a lot of knowledge in the process and I am very happy about that. My general comments in no particular order: 1) the patch is not in sync with trunk 2) when you comment out code because it is not needed anymore, just delete it instead. 3) when you comment out code because you don't know (yet) how to upgrade it with the new architecture, then put a FIXME explaining the issue and the potential solution. 4) we are a bunch of code rules fundamentalists so please read the docs in "trunk/development/coding/" and update your patch in consequence. 5) there are a certain number of cleanups that can be committed sequentially: the TextAnchor part, the TexMetrics part, etc. I believe we can integrate your patch step by step. 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. Having said that, I like very much the direction of your patch :-) I suggest that you 'try' to gain commit privilege and that you integrate your patch step by step by first posting the patch to the list so that I and others can comment easily. I say 'try' because I am not sure you would get it but please try anyway because I really don't feel like doing it for you. If someone else steps up that's fine too. Abdel.
Re: Scrollfix patch
Helge Hafting ha scritto: 6) I said that already but there's a number of optimisation that are gone now because you basically do a full screen update at each operation. Please, note that the updateFlags are still correctly produced by the various (quite obscure) code segments around. Except that they are ignored at the final rendering time. Actually, I didn't like the fact that, in the current trunk, AFAICU, the ViewMetricsInfo object is used, in the BufferView, as a kind of "reminder" of what needs to be refreshed. Then, in response to the same "request" (should be buffer_.changed()), the textMetrics.draw() method either draws the entire screen or just a paragraph depending on such "reminder". My next step would just be to rely on different signals, in the framework: -) one for updating the entire screen as a result of view-specific events, such as changing the anchor (i.e. scrolling), the zoom factor, what you see or hide, etc... -- this process should theoretically not interest the buffer anyway -) one for updating a single paragraph as a result of changing its contents (e.g. when you type characters) -- this should also trigger a redraw from the paragraph to the bottom if the height changes, of if neighbour paragraphs are affected as well (e.g. hitting enter to split a par into two) -- this process should probably start from the buffer itself, that should trigger a paragraph refresh of (all) the view(s) showing that buffer. This would eliminate the slowness (re)introduced by the current patch. I have had much trouble with performance. Still, I think that correctness is more important than performance. And the scrolling anomalies have been bad in some cases, especially short/medium sized documents containing big objects like full-page images. So correct scrolling beats fast but erratic scrolling. Still, I hope the optimizations comes back after a while. :-) Yes, this is the plan. T.
Re: Scrollfix patch
On Wed, Nov 07, 2007 at 12:04:46AM +0100, Tommaso Cucinotta wrote:
> Any comments by anyone is welcome.
It does not apply to trunk:
patching file src/ParagraphMetrics.h
patching file src/insets/InsetInclude.cpp
patching file src/insets/InsetText.cpp
patching file src/BufferView.cpp
Hunk #1 FAILED at 349.
Hunk #2 FAILED at 369.
Hunk #3 succeeded at 329 (offset -130 lines).
Hunk #4 succeeded at 385 (offset -141 lines).
Hunk #5 succeeded at 405 (offset -141 lines).
Hunk #6 succeeded at 443 (offset -142 lines).
Hunk #7 FAILED at 472.
Hunk #8 succeeded at 530 (offset -142 lines).
Hunk #9 succeeded at 640 (offset -142 lines).
Hunk #10 succeeded at 1123 with fuzz 1 (offset -166 lines).
Hunk #11 succeeded at 1135 (offset -164 lines).
Hunk #12 succeeded at 1289 (offset -162 lines).
Hunk #13 succeeded at 1324 (offset -162 lines).
Hunk #14 FAILED at 1336.
Hunk #15 succeeded at 1393 (offset -162 lines).
Hunk #16 succeeded at 1532 (offset -162 lines).
Hunk #17 FAILED at 1568.
Hunk #18 succeeded at 1579 (offset -166 lines).
Hunk #19 succeeded at 1600 (offset -166 lines).
Hunk #20 succeeded at 1612 (offset -166 lines).
Hunk #21 succeeded at 1635 (offset -166 lines).
Hunk #22 succeeded at 1651 (offset -166 lines).
Hunk #23 succeeded at 1785 (offset -166 lines).
Hunk #24 succeeded at 1827 (offset -166 lines).
Hunk #25 succeeded at 1836 (offset -166 lines).
Hunk #26 FAILED at 1848.
Hunk #27 succeeded at 2035 (offset -150 lines).
6 out of 27 hunks FAILED -- saving rejects to file src/BufferView.cpp.rej
patching file src/TextMetrics.h
patching file src/TextMetrics.cpp
Hunk #1 succeeded at 134 (offset 1 line).
Hunk #2 succeeded at 392 (offset 1 line).
Hunk #3 succeeded at 420 with fuzz 1 (offset 1 line).
Hunk #4 succeeded at 500 (offset 1 line).
Hunk #5 succeeded at 530 (offset 1 line).
Hunk #6 succeeded at 564 (offset 1 line).
Hunk #7 succeeded at 921 (offset 1 line).
Hunk #8 succeeded at 1124 (offset 1 line).
Hunk #9 succeeded at 1269 (offset 1 line).
Hunk #10 succeeded at 1309 (offset 1 line).
Hunk #11 succeeded at 1378 (offset 1 line).
Hunk #12 succeeded at 1405 (offset 1 line).
Hunk #13 succeeded at 1440 (offset 1 line).
Hunk #14 succeeded at 1464 (offset 1 line).
Hunk #15 succeeded at 1498 (offset 1 line).
Hunk #16 succeeded at 1543 (offset 1 line).
Hunk #17 succeeded at 1765 (offset 1 line).
Hunk #18 succeeded at 2010 (offset 1 line).
Hunk #19 succeeded at 2019 (offset 1 line).
Hunk #20 succeeded at 2133 (offset 3 lines).
Hunk #21 succeeded at 2278 (offset 3 lines).
patching file src/ParagraphMetrics.cpp
patching file src/frontends/WorkArea.h
patching file src/frontends/qt4/GuiView.cpp
patching file src/frontends/WorkArea.cpp
patching file src/Cursor.cpp
Hunk #1 succeeded at 1224 (offset 2 lines).
Hunk #2 succeeded at 1272 (offset 2 lines).
patching file src/BufferView.h
Hunk #6 succeeded at 211 with fuzz 2.
Hunk #7 succeeded at 219 (offset -1 lines).
Hunk #8 succeeded at 267 (offset -1 lines).
Hunk #9 succeeded at 302 (offset -3 lines).
patching file src/Text.h
> Index: src/ParagraphMetrics.h
> ===
> --- src/ParagraphMetrics.h(revisione 21411)
> +++ src/ParagraphMetrics.h(copia locale)
> @@ -113,6 +113,7 @@
> InsetDims inset_dims_;
> };
>
> +
> } // namespace lyx
Cosmetic changes please outside of significant changes like this one.
> +bool BufferView::fitCursor() {
> + return fitCursor(d->cursor_);
> +}
Brace on new line for function definitions.
> @@ -1342,7 +1289,8 @@
> case LFUN_SCREEN_UP:
> case LFUN_SCREEN_DOWN: {
> Point p = getPos(cur, cur.boundary());
> - if (p.y_ < 0 || p.y_ > height_) {
> + if (cursorStatus(cur) != CUR_INSIDE)
> + {
But not inside.
> Index: src/BufferView.h
> ===
> --- src/BufferView.h (revisione 21411)
> +++ src/BufferView.h (copia locale)
> @@ -19,6 +19,7 @@
>
> #include "support/strfwd.h"
> #include "support/types.h"
> +#include
Ok as long as the patch is developed, but I'd like something else in the
end.
Do you have any timing results (like loading the UserGuide before/after,
inserting a paragraph at the bin/middle/end of document)?
Andre'
Re: Scrollfix patch
Tommaso Cucinotta wrote: Index: src/ParagraphMetrics.h === --- src/ParagraphMetrics.h (revisione 21411) +++ src/ParagraphMetrics.h (copia locale) @@ -113,6 +113,7 @@ InsetDims inset_dims_; }; + } // namespace lyx #endif // PARAGRAPH_METRICS_H It helps make the patch readable if you just commit this kind of change, or remove it from your tree. Index: src/insets/InsetInclude.cpp === --- src/insets/InsetInclude.cpp (revisione 21411) +++ src/insets/InsetInclude.cpp (copia locale) @@ -277,7 +277,8 @@ case LISTINGS: temp = listings_label_; case NONE: - BOOST_ASSERT(false); + ; + //BOOST_ASSERT(false); } temp += : ; Same here. This isn't part of your patch. Otherwise, I don't really know this stuff. But if you can sort out scrolling...yeah rh -- == Richard G Heck, Jr Professor of Philosophy Brown University http://frege.brown.edu/heck/ == Get my public key from http://sks.keyserver.penguin.de Hash: 0x1DE91F1E66FFBDEC Learn how to sign your email using Thunderbird and GnuPG at: http://dudu.dyn.2-h.org/nist/gpg-enigmail-howto
Re: Scrollfix patch
Great! Will try it out later today. Stefan Am 07.11.2007 um 00:04 schrieb Tommaso Cucinotta: Hi Abdel, please find attached a first attempt of fixing the non-uniform scrolling of LyX on trunk (21411).
Re: Scrollfix patch
Tommaso Cucinotta wrote: Index: src/ParagraphMetrics.h === --- src/ParagraphMetrics.h (revisione 21411) +++ src/ParagraphMetrics.h (copia locale) @@ -113,6 +113,7 @@ InsetDims inset_dims_; }; + } // namespace lyx #endif // PARAGRAPH_METRICS_H It helps make the patch readable if you just commit this kind of change, or remove it from your tree. Index: src/insets/InsetInclude.cpp === --- src/insets/InsetInclude.cpp (revisione 21411) +++ src/insets/InsetInclude.cpp (copia locale) @@ -277,7 +277,8 @@ case LISTINGS: temp = listings_label_; case NONE: - BOOST_ASSERT(false); + ; + //BOOST_ASSERT(false); } temp += ": "; Same here. This isn't part of your patch. Otherwise, I don't really know this stuff. But if you can sort out scrolling...yeah rh -- == Richard G Heck, Jr Professor of Philosophy Brown University http://frege.brown.edu/heck/ == Get my public key from http://sks.keyserver.penguin.de Hash: 0x1DE91F1E66FFBDEC Learn how to sign your email using Thunderbird and GnuPG at: http://dudu.dyn.2-h.org/nist/gpg-enigmail-howto
Re: Scrollfix patch
Great! Will try it out later today. Stefan Am 07.11.2007 um 00:04 schrieb Tommaso Cucinotta: Hi Abdel, please find attached a first attempt of fixing the non-uniform scrolling of LyX on trunk (21411).
