search

hluk / CopyQ

Clipboard manager with advanced features
GNU General Public License v3.0
8.66k stars 441 forks source link

Support Mac OS X platforms #26

Closed hluk closed 10 years ago

hluk commented 11 years ago

Although code is supposed to run on OS X there is no developer for this platform.

TODO

Some window management functions are available in libqxt: https://bitbucket.org/libqxt/libqxt/src/master/src/widgets/mac/qxtwindowsystem_mac.cpp?at=master

rygwdn commented 10 years ago

Would love to see someone take this on, as I'll probably be switching to OSX + Linux soon. If no one takes it on before then (and I do end up switching), I'll do it :). Not sure I could work properly without CopyQ anymore!

hluk commented 10 years ago

If you (or someone) can implement the features that would be great. Everything is ready for implementation in src/platform/mac and hopefully it won't be very difficult.

I will extend the platform interface class as needed (e.g. we may now need something different than QSystemTray to show tray icon).

rygwdn commented 10 years ago

I've started looking into this on my mac-support branch. So far it builds, but it freezes after it launches (stuck in QSocketNotifer::event(QEvent*)).

The tray icon actually shows up nicely. I can even get the menu to show up if I pause and restart the application in the debugger.

hluk commented 10 years ago

Great work!

Glad to hear that the tray works, unlike the mean Unity on Ubuntu.

You can comment out line https://github.com/hluk/CopyQ/blob/f0cf81b8d93e65ac8c4d7e9abd4a5e368b93c3dc/src/app/clipboardserver.cpp#L103 to see if it works without running clipboard monitor process -- that's probably the weakest spot now.

rygwdn commented 10 years ago

Yeah, that fixes it. I actually came across that just before you replied! Any ideas how to fix that? Have you previously run into issues with the QLocalServer to make you think that's what was broken?

hluk commented 10 years ago

It's hard to say. I was fixing it some time ago because it misbehaved on Windows. Actually this is possibly what makes it unusable on OS/2 platform too (though CopyQ v1.6.3 worked well).

Can you post backtrace where it is stuck?

One place where it can get stuck is https://github.com/hluk/CopyQ/blob/f0cf81b8d93e65ac8c4d7e9abd4a5e368b93c3dc/src/app/remoteprocess.cpp#L71 (this part also needs to be modified so it's asynchronous). That's the server part; client part, i.e. clipboard monitor is here https://github.com/hluk/CopyQ/blob/f0cf81b8d93e65ac8c4d7e9abd4a5e368b93c3dc/src/app/clipboardmonitor.cpp#L264 .

So I guess QLocalSocket or QLocalServer is incorrectly set up.

rygwdn commented 10 years ago

I don't have my Mac with me at the moment, but the backtrace doesn't actually show any code from CopyQ after App::exec(). It just shows a QLocalServer doing an "accept" on a socket, which blocks the Qt event loop. That makes me think it's not in fact a blocking call somewhere inside CopyQ, but I'm not really sure what would cause it yet. I can post more details later.

rygwdn commented 10 years ago

I got it working. Using waitForNewConnection seems to cause things to break long after the connection has been made, so I modified that code to be async. I think it must have something to do with the socket being put into a blocking vs. non-blocking mode. There's still lots of work to do on the port though.

hluk commented 10 years ago

Great job!

I'll keep in mind that using *wait*() in Qt is not good (today I had some trouble with QFutureWatcher::waitForFinished() which actually passes thrown exceptions in the same manner as QFutureWatcher::result()).

rygwdn commented 10 years ago

Just a quick update on this. I'm still working on it, and so far there are 1579 modifications. It's a bit bigger than I expected!

Besides implementing the macplatform methods, here are a few of the unexpected issues I've run into so far:

  1. OS X doesn't trigger any event when the clipboard changes, so I had to add a timer to poll for changes
  2. As outlined above, OS X doesn't like mixing sync and async IO with sockets
  3. Not having a window open with a menu leads to all sorts of UI problems, so I had to disable that option in the UI, and I had to ensure that the main window is always open if any other window is open.
  4. In order to keep the CopyQ icon out of the dock when the UI isn't up, I've had to add a class (ForegroundBackgroundFilter) which changes the application state between "normal" and "background/prohibited".
  5. Qt on OS X doesn't allow you to add the same menu items to two menu bars, so I've had to duplicate the m_menuCommand member on the MainWindow class.
  6. OS X 10.9 doesn't allow "normal" apps to run in the background, so I've added a new class "MacActivity" which uses OS X APIs to create "activities" which allow the app to run in the background
  7. OS X doesn't use MIME types, they have their own "UTIs" instead. So I've added a new class to handle the mapping between UTIs and MIME types.

The code on my branch builds and seems to work well (if using qmake), but there are some pretty serious limitations (OS X 10.9 Mavericks with Qt 5.2).

hluk commented 10 years ago

OK, that's great!

I was checking commits in your branch a bit. Overall it looks good; I could start adding some comments now or wait for pull request. What do you think?

Do you know any CI service that lets us build for OS X (after a commit or sending source code package)?

rygwdn commented 10 years ago

I haven't started a pull request yet, because the branch is certainly not ready for merging! At the moment, almost all tests are failing, but I haven't yet been able to look into that (bigger issues have kept coming up). If it would make it easier, I could start a pull request so that we can have an easier conversation about the code, we just need to be careful not to actually merge it. What do you think?

As for CI services, it looks like Travis CI has an OS X build environment.

hluk commented 10 years ago

Yes, you can start pull request to mac-support branch.

Glad to see Travis supports OS X.

rygwdn commented 10 years ago

@hluk I've got a few questions which have come up working on this: 1) Why are there two ways to build CopyQ (qmake and cmake)? 2) Why are there generally two processes running (server and monitor)? 3) What is the code here doing? It seems to drop text if I copy a line of code, then copy that line and the next one.

I'm continuing to make progress on various aspects of the OS X support, and hope to have it to an alpha/beta ready state soon!

hluk commented 10 years ago

ad 1. qmake is there because it works well with Qt Creator and can build for Android platform quiet easily. cmake is there because it does everything else well :). It can generate necessary files pretty easily, install given files to custom locations and even though the documentation is nothing special I find it a lot better documented than qmake.

I'm open to any ideas concerning build systems.

ad 2. On Linux X11, an application holds clipboard data and provides them on request. So it can take quiet long time to retrieve clipboard if the clipboard provider is slow or doesn't respond at all. In the latter case you'll get either NULL or empty clipboard data with Qt (not sure which one is it). Additionally you have to retrieve data in GUI thread. So it's better if there is separate process for handling clipboard which can take longer time to respond or even crash.

ad 3. That's other peculiarity of X11. If you select a text it gets stored in primary selection (instead of regular clipboard). But this can happen immediately while selecting the text depending on application's behavior. So instead of creating item in CopyQ when each character or so is selected it tries to check if the text of the first item in list matches beginning or end of current selection. If it matches the first item just replaced without creating a new item. This works with regular clipboard too.

If this code doesn't work well or is unexpected, it should be disabled if the copied text doesn't come from X11 selection. What do you think?

hluk commented 10 years ago

I removed PlatformNativeInterface::getPasteWindow() (c461192b7da12bb0fd969f25b9fd910f1d891fd6). If window ID is different for raise and paste operations you should handle it when creating PlatformWindow using PlatformNativeInterface::getCurrentWindow().

Also see commit ea397682133f369325fba0a44c75d4ac7756e71f. If anything goes wrong when raising/focusing window (e.g. the window doesn't exist) the content must not be pasted.

rygwdn commented 10 years ago

Thanks @hluk. The OS X code already checks to see if the window successfully raised before pasting. I'll remove the getPasteWindow() method soon, it currently just returns getCurrentWindow() anyway.

I've been looking into the test failures, and I've found a few of the sources, and I'd like to get some feedback from you on some of them:

  1. When the tests set the clipboard, they use a monitor process to do so, but then the clipboard monitor checks to see if a monitor set the clipboard, and if so, doesn't send the data back to the UI, which causes this test to fail. I'm not sure about all the reasoning around all of that, and I'm even more confused by the fact that it works on other platforms.. what do you think?
  2. There are some shortcuts (Alt-1 & Down) that don't seem to work on OS X. I can work around these issues by using different shortcuts (Alt+Left & Tab). Should I just do so in an #ifdef Q_OS_MAC ?
  3. More insidious than the shortcuts above, "Backspace" is generally treated differently on OS X. I can easily make it always delete on OS X by changing the way it's handled, but I think it should be set as a default app shortcut instead? That unfortunately makes it even more complicated, because when setting a shortcut in the UI, "Backspace" is used to cancel setting the shortcut.. I'm not sure how to handle that gracefully. Maybe keep "Backspace" as the button to cancel setting a shortcut, and always handle "Backspace" to delete items in the UI?
  4. Having a delay on QTest::keyClick() causes a segfault when simulating "Enter". The tests seem to pass if I set the delay to 0, which leads me to believe that the widget to which "Enter" is being sent is somehow deleted while waiting. Could setting the delay to 0 be a problem elsewhere?
hluk commented 10 years ago

ad 1. Thanks for pointing out the location in code. That's quiet new code. It's there because otherwise if you run more CopyQ sessions they can fight over clipboard when trying to synchronize X11 selection and clipboard or they can start changing clipboard in response to user commands.

I didn't have time to check why one test is failing I always assumed that the interval after setting clipboard is too small.

I'll fix this.

ad 2. Yes, you can add #ifdefs.

ad 3. Backspace in main window resets and hides search entry field if it's not focused. I've never noticed it but there probably should be condition if (!browseMode()) { ... to check if user is searching. But Esc can be used too.

I guess you can remove the case Qt::Key_Backspace: in MainWindow and set backspace as additional key to delete item on OS X (perhaps at https://github.com/hluk/CopyQ/blob/master/src/gui/configtabshortcuts.cpp#L160).

I removed the restriction for Backspace key in shortcut dialog. So now you can override backspace and remove shortcut with dialog button (910760dfe7809ccb1aa95c8a64c8660ee8b73fa0).

ad 4. Damn. So QTest::keyClick() and QTest::keyClicks() are not safe. It would be better to use QTest::keyPress() and QTest::keyRelease() separately and check if the widget receiving events hasn't been deleted (using QPointer).

hluk commented 10 years ago

Commit 2ac473739a8d3a71b34d0a651e3ffcc171729627 should fix the failing tests.

rygwdn commented 10 years ago

From reading the docs, it looks to me like using QTest::keyClick with a delay just waits for delay ms before sending any events, it doesn't add a delay between press and release. Is that right? If so, is there any point in keeping the delay there?

rygwdn commented 10 years ago

Lots more work under a new PR.

The remaining work on this is now:

The first item there (weirdness from Finder), brings up another question for you. On OS X, when you copy a file in Finder, the clipboard/pasteboard gets:

My current thought on how to deal with this is that if the clipboard contains data for the mime type text/uri-list, then ignore everything else (plain text, images, etc.). Do you see this causing problems on other platforms?

rygwdn commented 10 years ago

Finished the first two items

hluk commented 10 years ago

Format display priority is strictly given by the order of item plugins in Preferences, Items tab. Each plugin checks if there is format it can display (creates ItemWidget).

I don't think that any icon is copied on Windows (Explorer) and Linux (Nautilus, Nemo ...) -- though other file manager can copy icons or anything else with URI lists.

I was thinking of creating a plugin for URI and this is a good reason to do so (https://github.com/hluk/CopyQ/issues/137).

hluk commented 10 years ago

On the QTest::keyClick() and QTest::keyClicks(): OK, you can remove the intervals -- it seems pretty useless, except it waits after key press to send a key release and it's actually nice to see what is happening when GUI tests run.

rygwdn commented 10 years ago

I think with that last merge, all that's left for OS X support is to get some testing, and figure out what else is broken that I don't use. Should we close this issue, and add new issues for anything that's found broken?

hluk commented 10 years ago

Alright. Thanks for bringing the app to OS X!