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

Support Mac OS X platforms #26

Closed
hluk opened this issue Apr 1, 2013 · 26 comments
Closed

Support Mac OS X platforms #26

hluk opened this issue Apr 1, 2013 · 26 comments
Labels

Comments

@hluk
Copy link
Owner

hluk commented Apr 1, 2013

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

TODO

  • Fix any compilation/linking issues in src/platform/mac/macplatform.cmake and src/platform/mac/macplatform.pri.
  • Implement PlatformNativeInterface in src/platform/mac/macplatform.cpp.

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

@rygwdn
Copy link
Collaborator

rygwdn commented Oct 25, 2013

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
Copy link
Owner Author

hluk commented Oct 25, 2013

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
Copy link
Collaborator

rygwdn commented Nov 27, 2013

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
Copy link
Owner Author

hluk commented Nov 27, 2013

Great work!

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

You can comment out line

startMonitoring();
to see if it works without running clipboard monitor process -- that's probably the weakest spot now.

@rygwdn
Copy link
Collaborator

rygwdn commented Nov 27, 2013

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
Copy link
Owner Author

hluk commented Nov 27, 2013

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

} else if ( !m_server->waitForNewConnection(16000) ) {
(this part also needs to be modified so it's asynchronous). That's the server part; client part, i.e. clipboard monitor is here
if ( !m_socket->waitForConnected(2000) )
.

So I guess QLocalSocket or QLocalServer is incorrectly set up.

@rygwdn
Copy link
Collaborator

rygwdn commented Nov 27, 2013

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
Copy link
Collaborator

rygwdn commented Nov 28, 2013

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
Copy link
Owner Author

hluk commented Nov 28, 2013

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
Copy link
Collaborator

rygwdn commented Dec 7, 2013

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
Copy link
Owner Author

hluk commented Dec 7, 2013

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
Copy link
Collaborator

rygwdn commented Dec 7, 2013

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
Copy link
Owner Author

hluk commented Dec 8, 2013

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

Glad to see Travis supports OS X.

@rygwdn rygwdn mentioned this issue Dec 8, 2013
10 tasks
@rygwdn
Copy link
Collaborator

rygwdn commented Dec 18, 2013

@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
Copy link
Owner Author

hluk commented Dec 18, 2013

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
Copy link
Owner Author

hluk commented Dec 21, 2013

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

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

@rygwdn
Copy link
Collaborator

rygwdn commented Dec 22, 2013

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
Copy link
Owner Author

hluk commented Dec 22, 2013

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 (910760d).

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
Copy link
Owner Author

hluk commented Dec 22, 2013

Commit 2ac4737 should fix the failing tests.

@rygwdn
Copy link
Collaborator

rygwdn commented Dec 22, 2013

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
Copy link
Collaborator

rygwdn commented Dec 23, 2013

Lots more work under a new PR.

The remaining work on this is now:

  • Fix weirdness around how files are copied by Finder (the OS X file manager)
  • Update documentation about OS X support (how to build, supported versions, etc.)
  • More testing
  • Set up Travis CI (if possible)

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:

  • plain text with the file name
  • a list of URIs
  • the icon for the file in a bunch of different image formats. When I say "icon" I don't mean thumbnail, I mean that if I copy a .png file, I get a bunch of images that say .png and have a generic picture on them.

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
Copy link
Collaborator

rygwdn commented Dec 23, 2013

Finished the first two items

@hluk
Copy link
Owner Author

hluk commented Dec 24, 2013

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 (#137).

@hluk
Copy link
Owner Author

hluk commented Dec 24, 2013

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
Copy link
Collaborator

rygwdn commented Dec 26, 2013

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
Copy link
Owner Author

hluk commented Dec 27, 2013

Alright. Thanks for bringing the app to OS X!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants