rbreu / beeref

BeeRef Reference Image Viewer
GNU General Public License v3.0
501 stars 37 forks source link

Enable opening files from Finder on MacOS X #17

Closed andrsd closed 3 years ago

andrsd commented 3 years ago

With this code, MacOS X user can double click .bee file and it gets opened in BeeRef.

andrsd commented 3 years ago

This is just a start/preview of code that enables MacOS X users to open .bee files from Finder.

There are 2 things to resolve, but I wanted to start the discussion and see what you think @rbreu

andrsd commented 3 years ago

I fixed the formatting, so the flake8 check should pass, now.

When I tried to run pytest . some tests ran, but they got stuck on tests/test_main.py where a window popped up and stayed there. Closing the window crashed the test suite run, so maybe we could see what the github testing has to say.

Also, I think I should write a test where we send the FileOpenEvent to the application, so we make sure this works. I did not work with pytest before, but I was looking over the test suite and I think I get the idea. Any guidance on where I should put the test would be highly appreciated. Thank you!

andrsd commented 3 years ago

I am not able to do the test :-( PyQt6.QtGui.QFileOpenEvent cannot be instantiated. And it would probably need Mac OS X test target since it is specific to that OS. Oh, well...

rbreu commented 3 years ago

I am not able to do the test :-( PyQt6.QtGui.QFileOpenEvent cannot be instantiated. And it would probably need Mac OS X test target since it is specific to that OS. Oh, well...

First, I wouldn't mind if left the test out (Qt stuff can be really tricky sometimes), but if you want to investigate further, this is what I would try (no guarantee that it works):

I'd put the test in tests/test_main.py — I usually maintain the same file structure from the actual code in the tests. In general, pytest-qt creates a QApplication instance and provides it for the unit tests since a lot of Qt classes can't be instantiated without an app running. You can tell pytest to always use the custom BeeRefApplication by putting this in tests/conftests.py (see docs):

@pytest.fixture(scope="session")
def qapp():
    yield CustomQApplication([])

Then your test would be something like this:

@patch('beeref.view.BeeGraphicsview.open_from_file') # mocking the open_from_file method
def test_beerefapplication_event(
        open_mock, # the mock of open_from_file
        qapp, # the pytest-fixture we have overridden that we are requesting here
        main_window, # another fixture defined in conftest.py that creates the BeeRefMainWindow
): 
    event = MagicMock()
    event.type.return_value = QtCore.QEvent.Type.FileOpen
    event.filename.return_value = 'test.bee'
    assert qapp.event(event) is True
    open_mock.assert_called_once_with('test.bee')

I have only ever run the tests on Linux. You might have more luck with running them with running only individual tests (pytest tests/main.py or pytest tests/test_main.py::test_beerefapplication_event, but if the issue is generally with opening windows within tests, that won't help you. :( Also unfortunately it looks like as long as you haven't made a commit into the repo yet, I need to manually start the workflows on your branch. Again, if this is too much trouble, we can leave the pull request as is.

andrsd commented 3 years ago

I was trying to do something like this:

def test_fileopen_event(qapp):
    root = os.path.dirname(__file__)
    filename = os.path.join(root, 'assets', 'test1item.bee')
    event = QtGui.QFileOpenEvent(filename)
    qapp.sendEvent(event)

But, I will try your suggestion. If it does not work out of the box, then we can leave this as is.

This is not too much trouble. I have a pyQt-based project where I want to use pytest so learning all this is very helpful and I appreciate your help. I work on apps that do not have GUI, so testing is much simpler. This is quite new to me, but I understand the importance of tests and having good code coverage. That's why I want to give it a try...

But, maybe it is a mute point, since you are testing only on linux and QFileOpenEvent is MacOS X specific (see docs)

andrsd commented 3 years ago

What you suggested seems to work. Had to change event.filename.return_value = 'test.bee' to event.file.return_value = 'test.bee'.

Let's give this another round though testing system. IDK what failed in the previous run, the tests ran for 1h 45mins, but I don't see any errors coming back.

rbreu commented 3 years ago

Let's give this another round though testing system. IDK what failed in the previous run, the tests ran for 1h 45mins, but I don't see any errors coming back.

It stalled again. I cloned your branch and tried it manually, and now I know what the issue is: The last test in test_main.py patches QApplication, but now needs to patch BeeRefApplication. :D So switch that line to:

@patch('beeref.__main__.BeeRefApplication')

Once you've changed this, I'm going to merge this. If I let the build workflow run on github and upload the macos build for you, would you give it a quick test?

andrsd commented 3 years ago

Yes, I can test the build.

Deploying binaries for Mac opens whole another can of worms. But, now that you switched to a single binary - you resolved a lot of problems. Maybe, I can post some more details on this topic into discussion if you are interested. I was playing with it during the last week or so.

andrsd commented 3 years ago

Also, the change you suggested on the last test in test_main.py caused that the test harness no longer sits there stuck on it and actually finishes cleanly for me, so yay!

rbreu commented 3 years ago

It's now merged. Thank you so much for doing this! The build from github is here:

https://www.dropbox.com/s/uu1k3edk2se3j7i/build-macos-10.15.zip?dl=0

andrsd commented 3 years ago

The feature we did here works if we give users the .app bundle. Just the binary itself does not work, since MacOS X is trying to find .app. The .app bundle is the standard way of distributing apps in MacOS X anyway, so we should be doing that instead.

One problem in the .app bundle is that Contents/MacOS/BeeRef-0.2.0 does not have the executable bit set and the bundle would not open. If I do chmod 755 Contents/MacOS/BeeRef-0.2.0, then it opens. I have to jump through the usual security hoops mentioned in #18 to open it, but then the app runs.

To solve the security problem, you would need to participate in the Apple Developer program (~$100 USD per year), sign the binary, have it notarized (automated process) and deploy a pkg or dmg. I tested all that and I was able to deploy a binary build via the --onefile option used by pyinstaller. Not suggesting you should do this, just what the solution is. I have seen other projects deploying unsigned binaries. Users do have a way to run them (hopefully that will still be the case with the new versions of OS X). I can put together some notes that would tell users what to do and where to click to get BeeRef running on MacOS X.

rbreu commented 3 years ago

Instructions would be amazing! I can set the executable flag in the build, no problem.

andrsd commented 3 years ago

I can set the executable flag in the build, no problem.

I tried the build locally and the binary has the executable flag set. So, I am wondering if it maybe got lost by uploading to dropbox? I did not play with the actions/upload-artifact@v2 github action, so IDK if it has some unwanted side effects. However, since .app bundle is a dir, I would use the usual approach and make a zip file and distribute that to users. But, I think we should make a small change in the spec file before you do a new release. I will explain that in a separate issue.

rbreu commented 3 years ago

I have uploaded a zip to dropbox and you can download it as is with the button in the topleft corner. That it shows you the zip content as a folder is just Dropbox being "helpful", and if you downloaded them individually, then yes, the file modes would get lost. But I just figured out that I change the link I can force the direct zip file download: https://www.dropbox.com/s/uu1k3edk2se3j7i/build-macos-10.15.zip?dl=1

But I don't have much time to investigate further at the moment since I'm about to go on vacation. I will get back to it when I get back.