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

Rtf popup QMainWindow #3

Closed
wants to merge 4 commits into from

Conversation

Holt59
Copy link
Contributor

@Holt59 Holt59 commented Jul 28, 2020

Based on frednet, tested:

  • Darnified UI - The README dialog is separate from the rest, which is pretty nice.
  • new_omod_2 with DisplayText - The popup is modal as expected (had to add a QEventLoop).
  • The other popups (yes/no, etc.), seem to be above the README, which is not very nice... I think that it is possible to avoid this by changing the parent of one or the other and switching to WindowModal.

@@ -8,18 +8,18 @@

#include "../interop/QtDotNetConverters.h"

RtfPopup::RtfPopup(System::String^ rtfText, QWidget* parent, Qt::WindowFlags f) : QDialog(parent, f)
RtfPopup::RtfPopup(System::String^ rtfText, QWidget* parent, Qt::WindowFlags f) : QMainWindow(nullptr, f)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we still take the parent argument if we're always using nullptr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the parameter and use it but now I pass nullptr explicitly when needed so that we can still pass other parents.

@AnyOldName3
Copy link
Owner

New set of questions now I've tried this out:

  • Without the size grip, it's harder to resize the popup on my touchscreen laptop. Is it something that can be brought back?
  • When the loading bar appears, it brings the main MO2 window on top of the readme, which is a pain. Can anything be done about it?

@AnyOldName3 AnyOldName3 mentioned this pull request Aug 3, 2020
@Holt59
Copy link
Contributor Author

Holt59 commented Aug 3, 2020

New set of questions now I've tried this out:

  • Without the size grip, it's harder to resize the popup on my touchscreen laptop. Is it something that can be brought back?

You can add a QStatusBar to the window, that will add a size grip. It's probably possible to do it without the status bar, but it looks like kind of complicated from Qt documentation...

  • When the loading bar appears, it brings the main MO2 window on top of the readme, which is a pain. Can anything be done about it?

I think the problem is that the Rtf popup does not have a parent. As you mentioned in the linked issue, we would probably need to create a custom hierarchy (with a "fake" widget) to fix this. Maybe it's not a bad idea to have the "init" dialog show in permanence and use it as parent for all dialog but the Rtf popup?

@AnyOldName3
Copy link
Owner

If I'm reading things correctly, keeping the init dialog around would only fix the DarNified UI stuff, not the readme stuff.

@Holt59
Copy link
Contributor Author

Holt59 commented Aug 4, 2020

It would make the readme popup stay on top of the main window but other dialogs will be put on top of it (but not modal).

@AnyOldName3
Copy link
Owner

I don't see any reason why it would. Can you explain (or open another PR proving me wrong)?

@Holt59
Copy link
Contributor Author

Holt59 commented Aug 4, 2020

That's how I understand Qt documentation.

Currently, the readme dialog has no parent, and it's Window modal, so it does not actually block anything and when a new Window modal dialog (or application modal) dialog with a parent is shown, everything is put on top of the readme dialog (the new dialog + the parent).

If we set the parent of the readme dialog to parentWidget() or something similar and keep it Windows modal, then the readme dialog will never be behind it.

You want the readme dialog and the init dialog to be "cousin", which may require adding a fake invisible widget in the hierarchy, and then have the other install dialogs be children of the init dialog, something like (maybe we don't need both fake widgets):

Parent Widget
    Fake Widget 1
        Readme Dialog
    Fake Widget 2
        Init dialog
            Other dialogs

@AnyOldName3
Copy link
Owner

I think I see what you're driving at now. With extra dummy widgets in the hierarchy, what you're saying makes more sense, but I thought you were suggesting an alternative to using dummy widgets.

I don't think we want the readme dialogue to actually be modal, though. For example, at least one mod I've got in my test suite installs, but doesn't try to activate, several optional ESP files, and what they do is described in the readme. Users will probably want to be able to enable them in the plugins tab while they still have the readme open. I think the ideal outcome would be if it floated on top of MO2 without being modal, a bit like running task manager with Always on top on, except obviously we'd want to let non-MO2 windows cover it.

popup.show();
QEventLoop loop;
connect(&popup, SIGNAL(closed()), &loop, SLOT(quit()), Qt::DirectConnection);
loop.exec();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New question. The popup's constructor sets WA_DeleteOnClose, but it's stack-allocated here. Is there anything guaranteeing we don't end up with a double-free?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never encountered issues with this when testing but I did not manage to find a clear answer for this in Qt documentation so it's probably better to move the setAttribute outside of the constructor.

@Holt59 Holt59 closed this Oct 21, 2020
AnyOldName3 pushed a commit that referenced this pull request Jun 5, 2021
Catch and rethrow .NET exceptions in OMODFrameworkWrapper constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants