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

Fixed a few issues with the XRef dialog #1384

Merged
merged 1 commit into from Mar 24, 2019

Conversation

Projects
None yet
6 participants
@AntonReborn
Copy link
Contributor

AntonReborn commented Mar 24, 2019

Fixes: #1337, #1368

The issue with the dialogs was in theirs constructing. If pass the parent to the dialog, it is created with the top left corner in (0; 0) of the parent. And in order to give it an offset QWidget::move(x, y) can be used. BTW, it's commonly used to create the different kinds of Pop-ups. The XRef dialog shouldn't have a parent. So just passing a nullptr for visibility.

@xarkes

xarkes approved these changes Mar 24, 2019

@AntonReborn

This comment has been minimized.

Copy link
Contributor Author

AntonReborn commented Mar 24, 2019

BTW, now the XRef dialog is modal within the Cutter, but not the whole OS. To make it clear, what is the expected behavior?

The possible problem with making it non-modal is that it can be easily loosed and the user will have to alt-tab to it. And even to use the touchpad on Mac, as alt-tab will not iterate through the different windows within an application.

cc @a1ext @ITAYC0HEN

@a1ext a1ext self-requested a review Mar 24, 2019

@@ -716,7 +716,7 @@ void DisassemblyContextMenu::on_actionSetFunctionVarTypes_triggered()

void DisassemblyContextMenu::on_actionXRefs_triggered()
{
XrefsDialog dialog(this);
XrefsDialog dialog(nullptr);

This comment has been minimized.

Copy link
@pelijah

This comment has been minimized.

Copy link
@AntonReborn

AntonReborn Mar 24, 2019

Author Contributor

I marked out it PR description, that I did it for visibility

This comment has been minimized.

Copy link
@pelijah

pelijah Mar 24, 2019

Contributor

And now it was merged to master as is...

@a1ext
Copy link
Member

a1ext left a comment

Tested on Windows and X-Refs dialog doesn't center on parent window, but it centers on the screen. I don't think this is the right behavior

@AntonReborn

This comment has been minimized.

Copy link
Contributor Author

AntonReborn commented Mar 24, 2019

@a1ext what you described breaks user experience. Try out the popular applications and look at dialog locations

@a1ext

This comment has been minimized.

Copy link
Member

a1ext commented Mar 24, 2019

@a1ext what you described breaks user experience. Try out the popular applications and look at dialog locations

Did you try IDA?

@a1ext

This comment has been minimized.

Copy link
Member

a1ext commented Mar 24, 2019

IDA always shows dialogs centered on the MainWindow:
image

@AntonReborn

This comment has been minimized.

Copy link
Contributor Author

AntonReborn commented Mar 24, 2019

@a1ext I talk about the majority of popular applications. This kind of argument is bad, when you are building a product which you want to be the №1.

@AntonReborn

This comment has been minimized.

Copy link
Contributor Author

AntonReborn commented Mar 24, 2019

@a1ext and if you want to copy IDA, this dialog on your screen-shot isn't in the main window center. It's shifted to a bottom

@Maijin

This comment has been minimized.

Copy link

Maijin commented Mar 24, 2019

For now I'd vote for @AntonReborn solution as it leave this to the user configuration instead of having Cutter taking responsibility on this.

We can always change it later if that approach is not liked by the users.

@a1ext

a1ext approved these changes Mar 24, 2019

Copy link
Member

a1ext left a comment

k, we discussed with guys and decided to leave it as you propose.

@ITAYC0HEN ITAYC0HEN merged commit e709a35 into radareorg:master Mar 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@a1ext

This comment has been minimized.

Copy link
Member

a1ext commented Mar 24, 2019

@AntonReborn Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.