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

Accept File And Folder Drops In Overlay Exceptions #3001

Merged
merged 5 commits into from Apr 23, 2017

Conversation

Projects
None yet
2 participants
@Kissaki
Member

Kissaki commented Apr 6, 2017

Implements #2998

Implemented in two commits. (The first commit prior to these is a fix, used in PR #3000.)

As described in the second commit, there's an open issue:

Issue: With an "Add" dialog open, dragging and dropping a file or folder
to where the dialog does not accept them will make the underlying widget
accept it.

The dialogs are modal QFileDialogs, so that is not the behaviour I would except. Dropping into a droppable area of the dialog (the file list area) captures the event as expected, with no bleeding into the window and widget below. With the dialog in front, and the mouse/drag events over non-droppable areas apparently propagates the mouse drag events to the window and widget below rather than discarding/blocking them.

I could make all four list widgets not accept drops before opening the dialog, and then making them accept them afterwards again, but that seems tedious; a lot of logic to fix unexpected (to me) behaviour. @mkrautz any input/ideas regarding this?

@Kissaki Kissaki requested a review from mkrautz Apr 6, 2017

@mkrautz

Can you refactor this to:

  • A commit refactoring applicaitonIdentifierForPath and application applicationInfoForId.
  • A commit for implementing and adding PathListWidget to the build
  • A commit for using PathListWidget in OverlayConfig

? It would make it seem much simpler.

Other things:

  • There are a few small coding-style problems:

    • Sometimes the * or & hugs the type, or there is whitespace in both directions, instead of hugging the variable.
    • Missing {} around single-line ifs (we try to adhere to this)
  • You're using the auto keyword, we still support C++98/C++03
    in 1.3.0.

  • mumble.pro has changed line-endings?

  • Is this a WIP? First commit still has comments in it, that the second commit removes.
    It seems like most of the changes in the second commit should be be merged into the
    "refactored" commits I mentioned at the beginning, so something like:

    • PathListWidget changes -> implement in original PathListWidget commit.
    • OverlayConfig changes -> implement in commit that implements PathListWidget in OverlayConfig.

Code looks good otherwise.

@Kissaki Kissaki changed the title from Accept File And Folder Drops In Overlay Exceptions to WIP: Accept File And Folder Drops In Overlay Exceptions Apr 8, 2017

@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 8, 2017

Member

I talked with @mkrautz directly again about the file dialog not catching drops in all areas. He suggested not using a native file dialog for the file dialog (via dialog options), as it seems to be a Qt bug.

Member

Kissaki commented Apr 8, 2017

I talked with @mkrautz directly again about the file dialog not catching drops in all areas. He suggested not using a native file dialog for the file dialog (via dialog options), as it seems to be a Qt bug.

@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 18, 2017

Member

Using QFileDialog::DontUseNativeDialog did not fix it.

Maybe setting the window parent does?


It seems my test did not effectively use my code changes/assumptions. After setting up a small test project I was able to verify the non-native dialog works (correctly catches drops). I then verified in fact the Qt Creator file open dialog also does not correctly catch drop events.

QTBUG-60269

Member

Kissaki commented Apr 18, 2017

Using QFileDialog::DontUseNativeDialog did not fix it.

Maybe setting the window parent does?


It seems my test did not effectively use my code changes/assumptions. After setting up a small test project I was able to verify the non-native dialog works (correctly catches drops). I then verified in fact the Qt Creator file open dialog also does not correctly catch drop events.

QTBUG-60269

Kissaki added some commits Apr 20, 2017

Move OverlayAppInfo (creation) logic into OverlayAppInfo
Make OverlayAppInfo constructor private to force use of static utility functions
@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 20, 2017

Member

I would like to test now, but my Overlay currently does not show anywhere anymore - even on current Snapshot.

Can add multiple paths and files by dropping them. Adds only adequate entries.

I did not change the use of the native dialog, even with the issue. The issue exists in Qt Creator as well, and probably every other place we use the QFileDialog. I'd rather keep this for now, instead of making the file dialog inconsistent with (potential) other places we use one (by using the Qt dialog, which looks/is a bit different).

Member

Kissaki commented Apr 20, 2017

I would like to test now, but my Overlay currently does not show anywhere anymore - even on current Snapshot.

Can add multiple paths and files by dropping them. Adds only adequate entries.

I did not change the use of the native dialog, even with the issue. The issue exists in Qt Creator as well, and probably every other place we use the QFileDialog. I'd rather keep this for now, instead of making the file dialog inconsistent with (potential) other places we use one (by using the Qt dialog, which looks/is a bit different).

@Kissaki Kissaki changed the title from WIP: Accept File And Folder Drops In Overlay Exceptions to Accept File And Folder Drops In Overlay Exceptions Apr 20, 2017

@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 21, 2017

Member

After a restart I was able to test with a working overlay. All seems fine on Windows 10 x64. Untested on other systems.

@mkrautz PTAL

Member

Kissaki commented Apr 21, 2017

After a restart I was able to test with a working overlay. All seems fine on Windows 10 x64. Untested on other systems.

@mkrautz PTAL

@mkrautz

This comment has been minimized.

Show comment
Hide comment
@mkrautz

mkrautz Apr 21, 2017

Member

LGTM, but the build is failing on all OSes that are case-sensitive.
Seems like ui_Overlay.h tries to use lowercase <pathlistwidget.h> for some reason? Odd.

Member

mkrautz commented Apr 21, 2017

LGTM, but the build is failing on all OSes that are case-sensitive.
Seems like ui_Overlay.h tries to use lowercase <pathlistwidget.h> for some reason? Odd.

@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 22, 2017

Member

They were added in lowercase first (by Qt Creator), but I changed them to match our CamelCase naming. The generated include statement is lowercase on Windows as well.

I adjusted the file name in the "promote" dialog in Qt Creator (SO), which resulted in a native parameter on the widget and a customwidget declaration with adequate include filename.

Member

Kissaki commented Apr 22, 2017

They were added in lowercase first (by Qt Creator), but I changed them to match our CamelCase naming. The generated include statement is lowercase on Windows as well.

I adjusted the file name in the "promote" dialog in Qt Creator (SO), which resulted in a native parameter on the widget and a customwidget declaration with adequate include filename.

@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 22, 2017

Member

I don't know what the native parameter specifies…

Member

Kissaki commented Apr 22, 2017

I don't know what the native parameter specifies…

Kissaki added some commits Apr 20, 2017

Add PathListWidget with drop functionality
PathListWidget is a ListWidget with switchable file or folder drop-functionality
Use PathListWidget for Overlay exception lists
Allows dropping executable files for launchers, whitelist and blacklist,
and folders for the paths list.

Fix custom widget include file
Code formatting
Add curly braces on online conditions on existing code that was moved.
@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 22, 2017

Member

Squashed the fix commit into edcdc4d

Member

Kissaki commented Apr 22, 2017

Squashed the fix commit into edcdc4d

@mkrautz

This comment has been minimized.

Show comment
Hide comment
@mkrautz

mkrautz Apr 23, 2017

Member

Still LGTM

Member

mkrautz commented Apr 23, 2017

Still LGTM

@Kissaki Kissaki merged commit 2968a92 into mumble-voip:master Apr 23, 2017

2 checks passed

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

@Kissaki Kissaki deleted the Kissaki:olay-exceptions-droppable branch Apr 23, 2017

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