Version: 1.2.0_devel (using KDE KDE 3.4.0) OS: Linux It would be nice if there were an easier way to setup axis supression on a group of plots. As it stands you have to edit each plot individually and set the supression depending on whether that specific plot is in the middle, or at a boundry of the group. I'm not sure about the best UI for this, though. Perhaps a button that automatically does all possible grouping in a single window based on the range of all the axes? Or a way to indicate what grouping you want (selection of plots?)?
Should be fixed for 1.2.1 release
This sounds like a layout mode action: -Select a bunch of plots -RMB->condense X Axis or -RMB->condense Y Axis It will find all stacked and selected windows, and set the surpress axis features appropriately (eg, top ones keep the top, bottom ones keep the bottom, middle ones lose both). It will not call cleanup layout, in case the user has done some fancy layouts. Thoughts?
On Fri, Jun 16, 2006 at 09:45:56PM -0000, netterfield@astro.utoronto.ca wrote: > This sounds like a layout mode action: > -Select a bunch of plots > -RMB->condense X Axis > or > -RMB->condense Y Axis > > It will find all stacked and selected windows, and set the surpress > axis features appropriately (eg, top ones keep the top, bottom ones > keep the bottom, middle ones lose both). It will not call cleanup > layout, in case the user has done some fancy layouts. > > Thoughts? I like it. Alot.
I don't use that feature very much but the proposal sounds good :-)
Created attachment 16732 [details] Proposed patch
I have a lot of comments about this patch. To start: 1) I don't like the fact that this is in ksttoplevelview. I'm not entirely sure where it should go yet. If we are going to have plot management code at a global level, maybe it should be implemented in a manager class. 2) Don't iterate and kst_cast<> the whole selection list when all you care about is two entries; kst_cast is expensive. 3) Creating a sub list already exists in the form of a template function kstObjectSubList<>. That should be used instead of home-grown sub-list extraction. I know it does unnecessary locking, but it allows us to change this code in all locations in one shot when needed - and it was needed many times in the past. 4) No object is ever supposed to paint() directly anymore. This is a serious policy violation. The paint should be done on the view widget itself.
Quoting Barth Netterfield <netterfield@physics.utoronto.ca>: > On one hand we could argue that it is like a special case of cleanup layout, > > and should go in the same place as the cleanup layout code. On the other > hand, we could argue that because it is 2dplot specific, it should exist > within 2dplot. 2dplot is a very core part of Kst still. If there is any object we could special case, it's this one. > In any case, it is very localized, whereever it goes, and can easily be > moved. > Would it be acceptable for items 2-4 to be fixed, and to then have it > commited so we can start to test the behavior of the feature, and then move > it to where it belongs once you decide (which would hopefully be soon)? Yes, I think so, as long as there is a plan to move this ~soon. I think the best solution is to make a page layout manager class (singleton) that can act on an object to layout its children according to various rules. > It will need javascript bindings, which we can't put in until it's proper > location has been decided. Exactly.
Created attachment 16744 [details] Proposed patch 1) Once you've made a decision let me know 2) I'm interested in all the entries 3) Done 4) Are you proposing widget()->paint() instead or posting an event?
Quoting Andrew Walker <arwalker@sumusltd.com>: > 1) Once you've made a decision let me know See the other comments on this report. > 2) I'm interested in all the entries numPlots is never used beyond "> 1" as far as I can see. > 4) Are you proposing widget()->paint() instead or posting an event? widget()->paint() is fine, at least for now. That's what's done elsewhere. Eventually it should be a paint event but I'm not sure how well that will work right now, so I haven't done it elsewhere in view objects either.
1) I don't see that any decision has been reached. If the proposed plan is to move everything to a manager class then I think a seperate bug report should be entered as it will impact much more than just this code. 2) I'm interested in all the entries 4) widget()->paint(QRegion()) calls KstWidget::paintEvent(...) calls KstTopLevelView::paint(KstPainter::P_PAINT); (with nothing else going on in any of those functions) where presently we're simply calling KstTopLevelView::paint(KstPainter::P_PAINT); At present all the places that a paint is triggered from KstTopLevelView calls the paint directly. If this should change for all cases then I think a separate bug report is needed.
Quoting Andrew Walker <arwalker@sumusltd.com>: > 1) I don't see that any decision has been reached. If the proposed plan is to > move everything to a manager class then I think a seperate bug report should > be entered as it will impact much more than just this code. It's not a bug or a user-facing feature, but you can file one if you want. > 2) I'm interested in all the entries The patch is blocked until this is cleared up, and this is not clear. Either the code is iterating and doing expensive dynamic casts before popping up a menu (which causes significant lag), or there is code missing in the patch. > 4) widget()->paint(QRegion()) calls > KstWidget::paintEvent(...) calls > KstTopLevelView::paint(KstPainter::P_PAINT); > (with nothing else going on in any of those functions) > where presently we're simply calling > KstTopLevelView::paint(KstPainter::P_PAINT); > > At present all the places that a paint is triggered from KstTopLevelView > calls the paint directly. Correct. And the proper way to do it is to paint the widget. Eventually all calls to paint the widget will be propagated up further into the app too. > If this should change for all cases then I think a > separate bug report is needed. It is something that will change eventually, but not now. It's not worth introducing new bugs by changing something that works. I don't want to propagate incorrect code either, so it has to use the new paint rules. This change in painting procedure was implemented and announced many months ago and needs to be followed.
SVN commit 553711 by arwalker: CCBUG:111239 Feature complete barring architectural requirements from George and additions to JS. M +147 -1 ksttoplevelview.cpp M +2 -0 ksttoplevelview.h --- trunk/extragear/graphics/kst/src/libkstapp/ksttoplevelview.cpp #553710:553711 @@ -1190,7 +1190,25 @@ menu->insertSeparator(); } - KPopupMenu *subMenu = new KPopupMenu(menu); + KPopupMenu *subMenu; + int numPlots = 0; + + for (KstViewObjectList::ConstIterator i = _selectionList.begin(); i != _selectionList.end(); ++i) { + if (kst_cast<Kst2DPlot>(*i)) { + numPlots++; + if (numPlots > 1) { + break; + } + } + } + if (numPlots > 1) { + subMenu = new KPopupMenu(menu); + subMenu->insertItem(i18n("X-axis"), this, SLOT(condenseXAxis())); + subMenu->insertItem(i18n("Y-axis"), this, SLOT(condenseYAxis())); + menu->insertItem(i18n("Condense"), subMenu); + } + + subMenu = new KPopupMenu(menu); subMenu->insertItem(i18n("Width"), this, SLOT(makeSameWidth())); subMenu->insertItem(i18n("Height"), this, SLOT(makeSameHeight())); subMenu->insertItem(i18n("Size"), this, SLOT(makeSameSize())); @@ -1231,6 +1249,134 @@ } +void KstTopLevelView::condenseXAxis() { + if (_pressTarget) { + KstApp::inst()->document()->setModified(); + Kst2DPlotList plots; + Kst2DPlotList plotsProcess; + QRect geom; + bool processed = false; + + // create a list of all plot objects in this view + plots = kstObjectSubList<KstViewObject, Kst2DPlot>(_selectionList); + + while (plots.count() > 1) { + plotsProcess.clear(); + plotsProcess.append(plots.first()); + geom = plots.first()->geometry(); + plots.pop_front(); + bool added = true; + + // find all the plots that are attached to this one + while (added && plots.count() > 0) { + added = false; + + for (Kst2DPlotList::ConstIterator i = plots.begin(); i != plots.end(); ++i) { + if ((*i)->geometry().left() == geom.left() && (*i)->geometry().right() == geom.right()) { + if (geom.top() - (*i)->geometry().bottom() == 1) { + geom = geom.unite((*i)->geometry()); + plotsProcess.append(*i); + plots.remove(*i); + added = true; + break; + } else if ((*i)->geometry().top() - geom.bottom() == 1) { + geom = geom.unite((*i)->geometry()); + plotsProcess.prepend(*i); + plots.remove(*i); + added = true; + break; + } + } + } + } + + // modify the plot properties appropriately + if (plotsProcess.count() > 1) { + plotsProcess.first()->setSuppressTop(true); + plotsProcess.pop_front(); + plotsProcess.last()->setSuppressBottom(true); + plotsProcess.pop_back(); + + for (Kst2DPlotList::Iterator i = plotsProcess.begin(); i != plotsProcess.end(); ++i) { + (*i)->setSuppressTop(true); + (*i)->setSuppressBottom(true); + } + + processed = true; + } + } + + if (processed) { + widget()->paint(QRegion()); + } + } +} + + +void KstTopLevelView::condenseYAxis() { + if (_pressTarget) { + KstApp::inst()->document()->setModified(); + Kst2DPlotList plots; + Kst2DPlotList plotsProcess; + QRect geom; + bool processed = false; + + // create a list of all plot objects + plots = kstObjectSubList<KstViewObject, Kst2DPlot>(_selectionList); + + while (plots.count() > 1) { + plotsProcess.clear(); + plotsProcess.append(plots.first()); + geom = plots.first()->geometry(); + plots.pop_front(); + bool added = true; + + // find all the plots that are attached to this one + while (added && plots.count() > 0) { + added = false; + + for (Kst2DPlotList::ConstIterator i = plots.begin(); i != plots.end(); ++i) { + if ((*i)->geometry().top() == geom.top() && (*i)->geometry().bottom() == geom.bottom()) { + if (geom.left() - (*i)->geometry().right() == 1) { + geom = geom.unite((*i)->geometry()); + plotsProcess.append(*i); + plots.remove(*i); + added = true; + break; + } else if ((*i)->geometry().left() - geom.right() == 1) { + geom = geom.unite((*i)->geometry()); + plotsProcess.prepend(*i); + plots.remove(*i); + added = true; + break; + } + } + } + } + + // modify the plot properties appropriately + if (plotsProcess.count() > 1) { + plotsProcess.first()->setSuppressLeft(true); + plotsProcess.pop_front(); + plotsProcess.last()->setSuppressRight(true); + plotsProcess.pop_back(); + + for (Kst2DPlotList::Iterator i = plotsProcess.begin(); i != plotsProcess.end(); ++i) { + (*i)->setSuppressLeft(true); + (*i)->setSuppressRight(true); + } + + processed = true; + } + } + + if (processed) { + widget()->paint(QRegion()); + } + } +} + + void KstTopLevelView::makeSameWidth() { if (_pressTarget) { KstApp::inst()->document()->setModified(); --- trunk/extragear/graphics/kst/src/libkstapp/ksttoplevelview.h #553710:553711 @@ -81,6 +81,8 @@ private slots: void menuClosed(); + void condenseXAxis(); + void condenseYAxis(); void makeSameWidth(); void makeSameHeight(); void makeSameSize();
Nice. But there are some issues with the behavior: For X: When you select a complete grid all is well, but if any of the selected plots have an un-selected plot on the same row, then it is not modified. This prevents you from selecting a single column to collapse. It also prevents you from collapsing any plots in an incomplete row (say you have 10 plots in three columns). I think that the rule for X should be: if a plot is selected, and there is a selected plot above or below it, which is at least roughly aligned with it (say the corners are within ~5% of the width or so) then it should condense axis with that plot. The rule for Y should be analogous. If the user doesn't want the axis condensed (say, because then it wouldn't line up with its neighbour) then the user should feel free to not select it. Also, it would be good to always have the entry in the layout mode rmb menu (even if no plots have been selected - in which case it should be there, but disabled). The string could be changed from "Condense" to "Condense selected" to give a hint to the user.
SVN commit 553763 by arwalker: CCBUG:111239 Disable menu item rather than hide it when not applicable. Ensure update under all conditions. M +11 -5 ksttoplevelview.cpp --- trunk/extragear/graphics/kst/src/libkstapp/ksttoplevelview.cpp #553762:553763 @@ -1192,6 +1192,7 @@ KPopupMenu *subMenu; int numPlots = 0; + int id; for (KstViewObjectList::ConstIterator i = _selectionList.begin(); i != _selectionList.end(); ++i) { if (kst_cast<Kst2DPlot>(*i)) { @@ -1201,11 +1202,12 @@ } } } - if (numPlots > 1) { - subMenu = new KPopupMenu(menu); - subMenu->insertItem(i18n("X-axis"), this, SLOT(condenseXAxis())); - subMenu->insertItem(i18n("Y-axis"), this, SLOT(condenseYAxis())); - menu->insertItem(i18n("Condense"), subMenu); + subMenu = new KPopupMenu(menu); + subMenu->insertItem(i18n("X-axis"), this, SLOT(condenseXAxis())); + subMenu->insertItem(i18n("Y-axis"), this, SLOT(condenseYAxis())); + id = menu->insertItem(i18n("Condense selected"), subMenu); + if (numPlots < 2) { + menu->setItemEnabled(id, false); } subMenu = new KPopupMenu(menu); @@ -1293,13 +1295,17 @@ // modify the plot properties appropriately if (plotsProcess.count() > 1) { plotsProcess.first()->setSuppressTop(true); + plotsProcess.first()->setDirty(); plotsProcess.pop_front(); + plotsProcess.last()->setSuppressBottom(true); + plotsProcess.last()->setDirty(); plotsProcess.pop_back(); for (Kst2DPlotList::Iterator i = plotsProcess.begin(); i != plotsProcess.end(); ++i) { (*i)->setSuppressTop(true); (*i)->setSuppressBottom(true); + (*i)->setDirty(); } processed = true;
I think I see what you're trying to achieve, but the reasoning is not clear to me: why would you want to merge axes on plots that are not touching? why would you want to merge axes on plots that do not have the same horizontal extent? It would seem simple enough for the user to cleanup layout to ensure they do. what if plots are overlapping vertically? Which axes are hidden? What if you have a series of, say 10, plots which are left-aligned in a single column but whose widths increase by 5% with each plot? Which ones are included in the codensation?
On Wednesday 21 June 2006 20:43, Andrew Walker wrote: > ------- I think I see what you're trying to achieve, but the reasoning is > not clear to me: sorry.... here we go.... > why would you want to merge axes on plots that are not touching? we don't - but if some one has arranged them by hand, and is off by a few pixels, we want to be a little generous... maybe 5% of the width of the plot? Maybe less is ok too. > why would you want to merge axes on plots that do not have the same > horizontal extent? It would seem simple enough for the user to cleanup > layout to ensure they do. Unless the user has some plots wider than others. We do this. So as long as they are 'close' we should consider them the same. > > what if plots are overlapping vertically? Which axes are hidden? by a few pixels? accept it - its probably just imprecise plot positioning. By a lot? Don't accept it - they are doing somehting else. > What if you have a series of, say 10, plots which are left-aligned in a > single column but whose widths increase by 5% with each plot? Which ones > are included in the codensation? Maybe 5% is too generous. On the other hand, if they were selected, we should assume it was for a reason. If there is a plausable condensation partner, then we should condense. The biggest problem is: Make 10 plots, in three columns with the datawizard. In layout mode, select all 10, and condense X axis. Only the first 3 rows get condensed. I would expect that all of them would condense, so there would only be axis numbers on the bottom plot of each of the three columns. Or Make a grid of plots: select one column. Condense X axis. Nothing happens. The column we selected should be condensed. So, the rule should be, if there is a plausable selected plot to condense with, condense with it. cbn
SVN commit 553965 by arwalker: CCBUG:111239 Make everything suitably fuzzy for axes condensation so that the user can be off by a few pixels when aligning plots and still have the axes condense as expected. M +76 -15 ksttoplevelview.cpp --- trunk/extragear/graphics/kst/src/libkstapp/ksttoplevelview.cpp #553964:553965 @@ -1256,15 +1256,20 @@ KstApp::inst()->document()->setModified(); Kst2DPlotList plots; Kst2DPlotList plotsProcess; + KstAspectRatio aspect; + KstAspectRatio aspectItem; QRect geom; + QRect geomItem; bool processed = false; - + const double close = 0.02; + // create a list of all plot objects in this view plots = kstObjectSubList<KstViewObject, Kst2DPlot>(_selectionList); while (plots.count() > 1) { plotsProcess.clear(); plotsProcess.append(plots.first()); + aspect = plots.first()->aspectRatio(); geom = plots.first()->geometry(); plots.pop_front(); bool added = true; @@ -1274,22 +1279,46 @@ added = false; for (Kst2DPlotList::ConstIterator i = plots.begin(); i != plots.end(); ++i) { - if ((*i)->geometry().left() == geom.left() && (*i)->geometry().right() == geom.right()) { - if (geom.top() - (*i)->geometry().bottom() == 1) { - geom = geom.unite((*i)->geometry()); + aspectItem = (*i)->aspectRatio(); + geomItem = (*i)->geometry(); + + if ((geomItem.left() == geom.left() && geomItem.right() == geom.right()) || + (fabs(aspectItem.x - aspect.x) < close && fabs(aspectItem.w - aspect.w) < close)) { + if (geom.top() - geomItem.bottom() == 1) { + geom = geom.unite(geomItem); plotsProcess.append(*i); plots.remove(*i); added = true; break; - } else if ((*i)->geometry().top() - geom.bottom() == 1) { - geom = geom.unite((*i)->geometry()); + } else if (geomItem.top() - geom.bottom() == 1) { plotsProcess.prepend(*i); - plots.remove(*i); + plots.remove(*i); added = true; break; + } else if (fabs(aspect.y - (aspectItem.y + aspectItem.h)) < close) { + plotsProcess.append(*i); + plots.remove(*i); + added = true; + break; + } else if (fabs(aspectItem.y - (aspect.y + aspect.h)) < close) { + plotsProcess.prepend(*i); + plots.remove(*i); + added = true; + break; } } } + + if (added) { + geom = geom.unite(geomItem); + + if (aspect.y < aspectItem.y) { + aspect.y = aspectItem.y; + } + if (aspect.y + aspect.h > aspectItem.y + aspectItem.h) { + aspect.h = (aspectItem.y + aspectItem.h) - aspect.y; + } + } } // modify the plot properties appropriately @@ -1324,15 +1353,20 @@ KstApp::inst()->document()->setModified(); Kst2DPlotList plots; Kst2DPlotList plotsProcess; + KstAspectRatio aspect; + KstAspectRatio aspectItem; QRect geom; + QRect geomItem; bool processed = false; - + const double close = 0.02; + // create a list of all plot objects plots = kstObjectSubList<KstViewObject, Kst2DPlot>(_selectionList); while (plots.count() > 1) { plotsProcess.clear(); plotsProcess.append(plots.first()); + aspect = plots.first()->aspectRatio(); geom = plots.first()->geometry(); plots.pop_front(); bool added = true; @@ -1342,40 +1376,67 @@ added = false; for (Kst2DPlotList::ConstIterator i = plots.begin(); i != plots.end(); ++i) { - if ((*i)->geometry().top() == geom.top() && (*i)->geometry().bottom() == geom.bottom()) { - if (geom.left() - (*i)->geometry().right() == 1) { - geom = geom.unite((*i)->geometry()); + aspectItem = (*i)->aspectRatio(); + geomItem = (*i)->geometry(); + + if ((geomItem.top() == geom.top() && geomItem.bottom() == geom.bottom()) || + (fabs(aspectItem.y - aspect.y) < close && fabs(aspectItem.h - aspect.h) < close)) { + if (geom.left() - geomItem.right() == 1) { plotsProcess.append(*i); plots.remove(*i); added = true; break; - } else if ((*i)->geometry().left() - geom.right() == 1) { - geom = geom.unite((*i)->geometry()); + } else if (geomItem.left() - geom.right() == 1) { plotsProcess.prepend(*i); plots.remove(*i); added = true; break; + } else if (fabs(aspect.x - (aspectItem.x + aspectItem.w)) < close) { + plotsProcess.append(*i); + plots.remove(*i); + added = true; + break; + } else if (fabs(aspectItem.x - (aspect.x + aspect.w)) < close) { + plotsProcess.prepend(*i); + plots.remove(*i); + added = true; + break; } } } + + if (added) { + geom = geom.unite(geomItem); + + if (aspect.x < aspectItem.x) { + aspect.x = aspectItem.x; + } + if (aspect.x + aspect.w > aspectItem.x + aspectItem.w) { + aspect.w = (aspectItem.x + aspectItem.w) - aspect.x; + } + } } // modify the plot properties appropriately if (plotsProcess.count() > 1) { plotsProcess.first()->setSuppressLeft(true); + plotsProcess.first()->setDirty(); plotsProcess.pop_front(); + plotsProcess.last()->setSuppressRight(true); + plotsProcess.last()->setDirty(); plotsProcess.pop_back(); - + for (Kst2DPlotList::Iterator i = plotsProcess.begin(); i != plotsProcess.end(); ++i) { (*i)->setSuppressLeft(true); (*i)->setSuppressRight(true); + (*i)->setDirty(); } processed = true; } } - + if (processed) { widget()->paint(QRegion()); }
Barth, if the latest changes were acceptable to you I'll mark this as FIXED.
A quick look showed some weird bugs - I'm on vacation now, but will look more closely as soon as I can... or you can... cbn On Wednesday 28 June 2006 18:18, Andrew Walker wrote: [bugs.kde.org quoted mail]
Fixed in the summer by Andrew... any further quibbles can be re-opened.