Abdelrazak Younes wrote:
> Peter Kümmel wrote:
>> Bo Peng wrote:
>>>> But when you prefer the (-1,-1) solution I'll change it.
>>> Since (-1, -1) will save a significant amount of coding, I prefer this.
>>>
>>> Bo
>>
>> Here the patch with above changes.
>
> Hello Peter,
>
> Thanks for the updated patch and sorry for the inconvenience that my
> merging activity has caused to you.
>
No problem, thanks for your hard work improving the code base.
> Please find my comments below. Don't take them personally please and
> feel free to contradict me if you think I'm wrong.
>
>
>> I've also implemented the patch for the qt3 frontend.
>> Now Qt3 and Qt4 are an par on Linux and Windows.
>> Diff against current svn.
>>
>> Peter
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: src/frontends/qt3/lyx_gui.C
>> ===================================================================
>
>> - boost::shared_ptr<QtView> view_ptr(new QtView(width, height));
>> + boost::shared_ptr<QtView> view_ptr(new QtView);
>
> Note that in my next cleanup round I will move the QtView creation from
> lyx_gui to GuiImplementation.
>
>> LyX::ref().addLyXView(view_ptr);
>>
>> QtView & view = *view_ptr.get();
>>
>> - if (posx != -1 && posy != -1)
>> - view.move(QPoint(posx, posy));
>> + view.init();
>>
>> + if (width != -1 && height != -1 && posx != -1 && posy != -1) {
>> + view.initNormalGeometry(QRect(posx, posy, width, height));
>> + view.resize(width, height);
>> + view.move(posx, posy);
>> + if (maximize)
>> + {
>> + view.show();
>> + view.setWindowState(Qt::WindowMaximized);
>> + }
>> + }
>> +
>> view.show();
>> - view.init();
>
> So you don't need LyXView::init() anymore or is it done elsewhere?
before the if (width ...
I think it makes more sense to initialize before showing and resizing.
>
>>
>> // FIXME: some code below needs moving
>>
>> Index: src/frontends/qt3/QtView.C
>> ===================================================================
>> --- src/frontends/qt3/QtView.C (revision 14157)
>> +++ src/frontends/qt3/QtView.C (working copy)
>>
>> @@ -55,14 +55,12 @@
>>
>>
>>
>> -QtView::QtView(unsigned int width, unsigned int height)
>> +QtView::QtView()
>> : QMainWindow(), LyXView(), commandbuffer_(0), frontend_(*this)
>> {
>> - resize(width, height);
>> -
>> qApp->setMainWidget(this);
>>
>> - bufferview_.reset(new BufferView(this, width, height));
>> + bufferview_.reset(new BufferView(this, width(), height()));
>
> I think there might be a problem there. BufferView needs the size of the
> WorkArea but width() and height() will give you the size of the window,
> including menubar and toolbars.
No, this is given by frameGeometry, see
http://doc.trolltech.com/4.1/geometry.html
same for Qt3
> But I guess this is OK now because a resize event will happen just
> afterward and readjust the BufferView to the proper WorkArea size. So I
> guess that passing 100x100 or whatever will produce the same effect.
>
>>
>> menubar_.reset(new QLMenubar(this, menubackend));
>> getToolbars().init();
>> @@ -157,17 +155,62 @@
>> return qApp->activeWindow() == this;
>> }
>>
>> +void QtView::initNormalGeometry(const QRect & g)
>
> +void QtView::resetGeometry(const QRect & g)
>
The functionality of most code here is to have a valid not-maximized geometry:
the normal geometry.
initNormalGeometry does not change the geometry of the window, it's part
of the workaround for the missing normal/maximized functionality on x11/qt3,
so I think 'normal' is still a good choice for the naming.
>> +{
>> + normalGeometry_ = g;
>> + maxWidth=QApplication::desktop()->width()-20;
>> +}
>>
>> +QRect QtView::qtViewGeometry() const
>
> +QRect QtView::getGeometry() const
>
Yes, that's better.
>> +{
>> + QRect rec;
>> + // setX/Y changes the size!
>> + rec.setX(frameGeometry().x());
>> + rec.setY(frameGeometry().y());
>> + rec.setWidth(geometry().width());
>> + rec.setHeight(geometry().height());
>> + return rec;
>> +}
>> +
>> +void QtView::resizeEvent(QResizeEvent *)
>> +{
>> + if (width() > maxWidth) {
>> + maxWidth = width();
>> + return;
>> + }
>> + if (frameGeometry().x() > 0)
>> + normalGeometry_ = qtViewGeometry();
>> +
>> + std::cout<<maxWidth-normalGeometry_.width()<<", resizeEvent
>> :"<<normalGeometry_.x()<<"\n";
>> +}
>> +
>> +void QtView::moveEvent(QMoveEvent *)
>> +{
>> + if (width() < maxWidth && frameGeometry().x() > 0)
>> + normalGeometry_ = qtViewGeometry();
>
> This is just for updating x and y position isn't it? Why do we need that
> by the way? Because you want to save this in the session info as well. I
> guess it make sense on Windows but won't this clash with Window manager
> that do this under X11?
>
Here I update the normal size and position which should be saved when
lyx is closed in a maximized status. normalGeometry_ must not be overwritten
by values of the maximized window, therefore the if.
X11/qt3 does not provide the normal size of a maximized windows, because of
this all the code here.
>
>> Index: src/frontends/qt4/lyx_gui.C
>> ===================================================================
>> --- src/frontends/qt4/lyx_gui.C (revision 14157)
>> +++ src/frontends/qt4/lyx_gui.C (working copy)
>> @@ -194,15 +194,22 @@
>> // this can't be done before because it needs the Languages object
>> initEncodings();
>>
>> - boost::shared_ptr<GuiView> view_ptr(new GuiView(width, height));
>> + boost::shared_ptr<GuiView> view_ptr(new GuiView);
>> +
>> LyX::ref().addLyXView(view_ptr);
>>
>> GuiView & view = *view_ptr.get();
>>
>> view.init();
>> -
>> - if (posx != -1 && posy != -1) {
>> +
>> + if (width != -1 && height != -1 && posx != -1 && posy != -1) {
>> +#ifndef Q_OS_WIN32
>> + // X11: use frameGeometry position
>> + view.setGeometry(0, 0, width, height);
>> + view.move(posx, posy);
>> +#else
>> view.setGeometry(posx, posy, width, height);
>> +#endif
>> if (maximize)
>> view.setWindowState(Qt::WindowMaximized);
>
> I don't like the #ifndef at all. Why do we need them? Why only for qt4?
>
X11/qt4 is broken, we only need the workaround on X11. Most ifdefs could
be removed, but then we have some useless additional calls on windows, but this
isn't a big problem.
>
>> }
>> Index: src/frontends/qt4/GuiView.h
>> ===================================================================
>> --- src/frontends/qt4/GuiView.h (revision 14157)
>> +++ src/frontends/qt4/GuiView.h (working copy)
>> @@ -48,8 +48,8 @@
>> class GuiView : public QMainWindow, public LyXView {
>> Q_OBJECT
>> public:
>> - /// create a main window of the given dimensions
>> - GuiView(unsigned int w, unsigned int h);
>> + /// create a main window
>> + GuiView();
>>
>> ~GuiView();
>>
>> @@ -89,6 +89,15 @@
>> protected:
>> /// make sure we quit cleanly
>> virtual void closeEvent(QCloseEvent * e);
>> +
>> +#ifndef Q_OS_WIN32
>> + ///
>> + virtual void resizeEvent(QResizeEvent * e);
>> +
>> + ///
>> + virtual void moveEvent(QMoveEvent * e);
>> +#endif
>
> Same here. IMO if these methods are really necessary define them in any
> case. The #ifdef code in GuiView.C should then be encapsulated in some
> helper functions somewhere that will set the geometry of QWidget
> depending on the platform. Please understand that I've worked hard to
> avoid special cases in general code and I don't want them to come back.
> X11/Mac/WIn specific code should go in helper functions.
>
There's no problem removing the ifdefs. But I would prefer to have the
ifdef in the closeEvent implementation, it could be removed when TT has
fixed the issue.
>
>> +
>> private:
>> /// focus the command buffer widget
>> void focus_command_widget();
>> @@ -112,6 +121,14 @@
>> static QMainWindow* mainWidget_;
>>
>> GuiImplementation frontend_;
>> +
>> +#ifndef Q_OS_WIN32
>> + ///
>> + QRect qtViewGeometry() const;
>
> + QRect getGeometry() const;
>
>> +
>> + ///
>> + QRect normalGeometry_;
>> +#endif
>
>
> remove the #ifndef please.
>
No problem.
>
>> };
>>
>> } // namespace frontend
>> Index: src/frontends/qt4/GuiView.C
>> ===================================================================
>> --- src/frontends/qt4/GuiView.C (revision 14157)
>> +++ src/frontends/qt4/GuiView.C (working copy)
>> @@ -70,7 +70,7 @@
>> } // namespace anon
>>
>>
>> -GuiView::GuiView(unsigned int width, unsigned int height)
>> +GuiView::GuiView()
>> : QMainWindow(), LyXView(), commandbuffer_(0), frontend_(*this)
>> {
>> mainWidget_ = this;
>> @@ -78,7 +78,8 @@
>> // setToolButtonStyle(Qt::ToolButtonIconOnly);
>> // setIconSize(QSize(12,12));
>>
>> - bufferview_.reset(new BufferView(this, width, height));
>> + // -geometry could set the width and hight
>> + bufferview_.reset(new BufferView(this, geometry().width(),
>> geometry().height()));
>
>
> My comments to qt3 version are also valid here.
>
>
>> menubar_.reset(new QLMenubar(this, menubackend));
>> connect(menuBar(), SIGNAL(triggered(QAction *)), this,
>> SLOT(updateMenu(QAction *)));
>> @@ -176,10 +177,44 @@
>> return qApp->activeWindow() == this;
>> }
>>
>> +#ifndef Q_OS_WIN32
>
> Please remove.
>
>>
>> +QRect GuiView::qtViewGeometry() const
>> +{
>> + QRect rec;
>> + // setX/Y changes the size!
>> + rec.setX(frameGeometry().x());
>> + rec.setY(frameGeometry().y());
>> + rec.setWidth(geometry().width());
>> + rec.setHeight(geometry().height());
>> + return rec;
>> +}
>> +
>> +void GuiView::resizeEvent(QResizeEvent *)
>> +{
>> + if (!isMaximized())
>> + normalGeometry_ = qtViewGeometry();
>> +}
>> +
>> +void GuiView::moveEvent(QMoveEvent *)
>> +{
>> + if (!isMaximized())
>> + normalGeometry_ = qtViewGeometry();
>> +}
>> +#endif
>> +
>> void GuiView::closeEvent(QCloseEvent *)
>> {
>> +#ifndef Q_OS_WIN32
>> + QRect geometry;
>> + if (isMaximized()) + geometry = normalGeometry_;
>> + else
>> + geometry = qtViewGeometry();
>> +#else
>> QRect geometry = normalGeometry();
>> +#endif
>> +
>> Session & session = LyX::ref().session();
>> // save windows size and position
>> session.saveSessionInfo("WindowWidth",
>> convert<string>(geometry.width()));
>> @@ -199,6 +234,10 @@
>> {
>> QMainWindow::setWindowTitle(qt_("LyX"));
>> QMainWindow::show();
>> +#ifndef Q_OS_WIN32
>> + if (!isMaximized())
>> + normalGeometry_ = qtViewGeometry();
>> +#endif
>> }
>>
>>
>> Index: src/lyx_main.C
>> ===================================================================
>> --- src/lyx_main.C (revision 14157)
>> +++ src/lyx_main.C (working copy)
>> @@ -170,7 +170,7 @@
>>
>>
>> LyX::LyX()
>> - : first_start(false)
>> + : first_start(false), geometryOption_(false)
>> {}
>>
>>
>> @@ -335,7 +335,14 @@
>> if (!val.empty())
>> posy = convert<int>(val);
>> }
>> +
>> + if (geometryOption_) {
>> + width = -1;
>> + height = -1;
>> + }
>> +
>> lyx_gui::start(batch_command, files, width, height, posx,
>> posy, maximize);
>> +
>> } else {
>> // Something went wrong above
>> quitLyX(false);
>> @@ -995,6 +1002,10 @@
>> std::map<string, cmd_helper>::const_iterator it
>> = cmdmap.find(argv[i]);
>>
>> + // check for X11 -geometry option
>> + if (argv[i] == string("-geometry"))
>> + geometryOption_ = true;
>> +
>
> I have read somewhere that QApplication already understand this option.
> How would this interact with your code here?
>
Qt4::QApplication handles the -geometry option in the ctor,
Qt3::QApplication handles it when calling setMainWidget.
So when we use Qt4 we must know if we should resize the window
after constructing, and therefore we must knowif
there was a geometry option.
By this way both front ends uses the same logic:
Construct with geoemtry size/pos (if any) and only resize/move
when there was no geometry option.
>
>> // don't complain if not found - may be parsed later
>> if (it == cmdmap.end())
>> continue;
>> Index: src/lyx_main.h
>> ===================================================================
>> --- src/lyx_main.h (revision 14157)
>> +++ src/lyx_main.h (working copy)
>> @@ -107,6 +107,10 @@
>> ///
>> typedef std::list<boost::shared_ptr<LyXView> > ViewList;
>> ViewList views_;
>> +
>> + /// + bool geometryOption_;
>> +
>> };
>>
>> #endif // LYX_MAIN_H
>
>
>
>
>
