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

Configurable drop target directory on windows #972

Merged
merged 3 commits into from
Dec 10, 2020

Conversation

albertony
Copy link

@albertony albertony commented Dec 6, 2020

Make drop target directory when --enable-drag-drop is active configurable with new option --drop-dir. Default is Windows Desktop as before.

Related to #969 and #970, in the sense that the improve the ability to run as a "portable app" (e.g. copy-deploy with local configuration).

Copy link
Member

@p12tic p12tic left a comment

Choose a reason for hiding this comment

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

Just a single comment, looks good otherwise. Thanks!

src/lib/platform/MSWindowsScreen.cpp Outdated Show resolved Hide resolved
@albertony
Copy link
Author

Pushed an update (mutable instead of const).

One thing I noticed: The OSXScreen has its own m_dropTarget member string, with a non-virtual getter to return it. Not sure why it is not virtual?

const std::string& getDropTarget() const { return m_dropTarget; }

@albertony
Copy link
Author

albertony commented Dec 10, 2020

Another small change: Increased log level of a few central messages. There are already quite some "noice" with info messages just when moving cursor between screens ("leaving screen", "entering screen", "switch from x to y" etc), so I though a larger operation such as transferring a file would then also qualify for a info message.

By the way, the added sleep in the "Improve drag&drop stability on windows" commit is rather dirty, but seems the following lines were already in the same category. I got a lot of DEBUG: failed to get drag file name from OLE errors, to such a degree that drag-drop were basically useless. Adding this tiny sleep seems to improve it considerably, although i sometimes still get the error.

@p12tic
Copy link
Member

p12tic commented Dec 10, 2020

Pushed an update (mutable instead of const).

One thing I noticed: The OSXScreen has its own m_dropTarget member string, with a non-virtual getter to return it. Not sure why it is not virtual?

const std::string& getDropTarget() const { return m_dropTarget; }

virtual automatically carries over for functions that override the virtual function from the base class. So it can be omitted in theory. In practice this is error prone and we should mark the overriding functions with override.

Copy link
Member

@p12tic p12tic left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

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