Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't impose event handling #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NicoG60
Copy link

@NicoG60 NicoG60 commented Dec 5, 2019

Hi,
I have several issues with that bit of code.
First, I think it is better to let people subclass and override this method if they need and not impose the F key to be bound to a fullscreen feature. Maybe they want another key to toggle fullscreen, maybe they need osg to get the F key event.

Then I spotted an issue that might lead to a segfault? Not sure though but here is my point in the comment.

if(event->key() == Qt::Key_F)
{
    static QSize g;
    static QMargins sMargins;

    if(parent() && parent()->isWidgetType())
    {
        QMainWindow* _mainwindow = dynamic_cast<QMainWindow*>(parent());

        if(_mainwindow)
        {
            // Bla bla bla, fullscreen stuff...
        }
    }
    else
    {
        showNormal();
        setMinimumSize(g);

        /*
        
        Why that ?
        
        If we get here, first that can mean parent() is a nullptr.
        if parent() == nullptr, then the program will fall in this branch and
        segfault.

        Then, parent is not necessarly a main window. Can be any widget.
        Doing a dynamic cast here can return nullptr if parent() is not a
        mainwindow and segfault again.

        */ 
        QMainWindow* _mainwindow = dynamic_cast<QMainWindow*>(parent());
        _mainwindow->setCentralWidget(this);

        if(layout())
            layout()->setContentsMargins(sMargins);

        qApp->processEvents();
        setMinimumSize(QSize(1, 1));
    }
}
else // not 'F' key
{
    // forward event to renderer
    m_renderer->keyPressEvent(event);
}

I think it is better to let people handle that fullscreen feature as they want. They can have really different way of organising that widget into their app and maybe they want to be able to have more control over that.

@mathieu
Copy link
Collaborator

mathieu commented Dec 16, 2019

Hi @NicoG60,

Thanks for the PR, the removed code should probably best be transferred to a sample where the osgQOpenGLWidget is the central widget of a QMainWindow ?

@NicoG60
Copy link
Author

NicoG60 commented Dec 16, 2019

Yeah I think that piece of code could be a good example with a mainwindow as a container.
Just I don't think it is relevant to bake it in the library itself.
I can provide a simple example for that if you want

@mathieu mathieu self-requested a review December 16, 2019 14:17
@mathieu
Copy link
Collaborator

mathieu commented Dec 16, 2019

I can provide a simple example for that if you want

That would be awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants