On Fri, Apr 22, 2016 at 03:56:11AM +0100, Guillaume Munch wrote: > Le 21/04/2016 21:04, Scott Kostyshak a écrit :
> > Guillaume, do you think it is reasonable to revert this commit with > > respect to 2.2.0? Even if we were able to quickly fix it, it seems this > > might be sensitive code. > > > > Also, since Guillaume had made some improvements to this commit, it > > might be nice to know if Bruce still sees the problem on 2.3-staging. > > Bruce, can you do the following? > > git checkout 2.3-staging > > and compile and test there to see if this particular problem occurs? > > After that, it is still better for you to test 2.2.0rc1 with the > > reversion for future testing as I detailed in the other email. > > > > Reverting is a possibility. Another solution is to consider the patch in > 2.3-staging: it works very differently so it is unlikely to suffer from > the same issue. A third solution is the attached patch. Let me explain > the latter. > > The issue with the current code probably comes from the lines that force > the calculation of the sizes before the widget is shown, which is the > only hackish part. Let's assume this is correct (which a test of the > attached patch or of 2.3-staging will confirm). Then I do not know a > cleaner way of forcing the calculation in this place. So it is best to > get rid of it (and remember it as a lesson on how cross-platform Qt is > tricky). > > With the attached patch, there is no forcing of the calculation, only a > plain update of the minimum size when the dialog is shown. The downside > is that the bug meant to be solved by the minimum size calculation still > manifests itself the first time the dialog is shown. Now, the following > happens: LyX remembers the dialog sizes across sessions. As a > consequence, in the subsequent sessions, the dialog is created with the > good size, even though its minimum size is incorrect again (which the > user probably does not notice). For this reason we can see it as a > workaround to the initial issue. I like the patch you attached. Although I understand your dissatisfaction that it doesn't completely fix one of the regressions you were targetting, it is nice to get rid of that hack and more importantly to solve the crash. Please commit this patch to master. Thanks, Scott > > Assuming again that the tests of the attached patch and of 2.3-staging are > satisfactory, I see three possibilities: > > * Backport the LyXToolBox from 2.3-staging into master > + Solves the issue entirely > + The code is simpler than the current fix and does not rely on the same > trick. > - There is still a small hack to avoid flicker, by calling > qApp->processEvents(), for which maybe we want more testing time. > > * Revert b5a2f1c7 and backport LyXToolBox into 2.2.1 > + More time to test LyXToolBox > - The problems are back in 2.2.0 > > * Apply the attached patch into master and backport LyXToolBox into > 2.2.1 or 2.2.2 > + More time to test LyXToolBox > - Workaround not perfect > > It all comes down to what devs think of the attached patch and of > 367a7a6d from 2.3-staging. > > I am still puzzled by the messages "no inset hit" reported by Bruce, if > somebody could explain that then I would be happy. > > > Guillaume > > From 6a35e0978921cde8c3f093f774bcb25793a4f395 Mon Sep 17 00:00:00 2001 > From: Guillaume Munch <g...@lyx.org> > Date: Fri, 22 Apr 2016 02:55:10 +0200 > Subject: [PATCH] Fix trouble with openbox, fluxbox > > http://thread.gmane.org/gmane.editors.lyx.devel/161725 > > These wms have trouble with the fix at b5a2f1c7, probably more precisely with > the trick to force the calculation of the actual sizes before the display > (layout->invalidate() and the code around it). > > This patch gets rid of the code that forces the calculation. As a consequence, > the minimum sizes are again incorrect the first time the window is shown. They > are only correct the second time the window is shown. Now here is the trick: > LyX > remembers the sizes of windows between sessions. Therefore, as soon as the > good > minimum size has been set, the good size is remembered for the next > session. Thus, in the following sessions, even though the minimum size is > incorrect the first time, the dialog still opens with the good size. So the > user > does not see the problem in practice, apart from the very first time. > > This is meant as a temporary workaround. > --- > src/frontends/qt4/GuiCitation.cpp | 44 > +++++++++++++++------------------------ > 1 file changed, 17 insertions(+), 27 deletions(-) > > diff --git a/src/frontends/qt4/GuiCitation.cpp > b/src/frontends/qt4/GuiCitation.cpp > index 16c0990..7ef508b 100644 > --- a/src/frontends/qt4/GuiCitation.cpp > +++ b/src/frontends/qt4/GuiCitation.cpp > @@ -153,6 +153,23 @@ void GuiCitation::showEvent(QShowEvent * e) > { > findLE->clear(); > availableLV->setFocus(); > + > + // Set the minimal size of the QToolbox. Without this, the size of the > + // QToolbox is only determined by values in the ui file (e.g. computed > by > + // qtcreator) and therefore causes portability and localisation issues. > Note > + // that the page widgets must have a layout with layoutSizeContraint = > + // SetMinimumSize or similar. > + QSize minimum_size = QSize(0,0); > + // Compute the max of the minimal sizes of the pages > + QWidget * page; > + for (int i = 0; (page = citationTB->widget(i)); ++i) > + minimum_size = minimum_size.expandedTo(page->minimumSizeHint()); > + // Add the height of the tabs > + if (citationTB->currentWidget()) > + minimum_size.rheight() += citationTB->height() - > + citationTB->currentWidget()->height(); > + citationTB->setMinimumSize(minimum_size); > + > DialogView::showEvent(e); > } > > @@ -625,33 +642,6 @@ bool GuiCitation::initialiseParams(string const & data) > citeCmds_ = documentBuffer().params().citeCommands(); > citeStyles_ = documentBuffer().params().citeStyles(); > init(); > - > - // Set the minimal size of the QToolbox. Without this, the size of the > - // QToolbox is only determined by values in the ui file (e.g. computed > by > - // qtcreator) and therefore causes portability and localisation issues. > In > - // the future, this should be integrated into a custom widget if plans > are > - // made to generalise such a use of QToolboxes. Note that the page > widgets > - // must have a layout with layoutSizeContraint = SetMinimumSize or > similar. > - if (!isVisible()) { > - // only reliable way to get the size calculations up-to-date > - show(); > - layout()->invalidate(); > - hide(); > - // this does not show any window since hide() is called before > - // relinquishing control > - } > - QSize minimum_size = QSize(0,0); > - // Compute the max of the minimal sizes of the pages > - QWidget * page; > - for (int i = 0; (page = citationTB->widget(i)); ++i) > - minimum_size = minimum_size.expandedTo(page->minimumSizeHint()); > - // Add the height of the tabs > - if (citationTB->currentWidget()) > - minimum_size.rheight() += citationTB->height() - > - citationTB->currentWidget()->height(); > - citationTB->setMinimumSize(minimum_size); > - updateGeometry(); > - > return true; > } > > -- > 2.1.4 >
signature.asc
Description: PGP signature
