Skip to content

Add support for macOS and iOS - #161

Open
somestupidgirl wants to merge 9 commits into
ScanTailor-Advanced:masterfrom
somestupidgirl:master
Open

Add support for macOS and iOS#161
somestupidgirl wants to merge 9 commits into
ScanTailor-Advanced:masterfrom
somestupidgirl:master

Conversation

@somestupidgirl

Copy link
Copy Markdown

Made these patches since I couldn't find an up to date version for mac and since I use my iPad Pro a lot for working with PDFs and stuff I figured I might as well port it over to iOS/iPadOS as well.

Please let me know if there are any glaring issues. :)

@ferdnyc ferdnyc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As I mentioned in a couple of review comments, I have no role in this project. You should feel free to ignore any- or everything here, if you prefer, as none of my drive-by feedback carries any official weight whatsoever.

Just getting that out there right from the start.

Comment thread ios/AppIcon.appiconset/Contents.json Outdated
@@ -0,0 +1,23 @@
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why are there two identical copies of this entire directory in the PR? (/ios/AppIcon.appiconset/ and /ios/Assets.xcassets/AppIcon.appiconset/)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was a bug in the icon generation script. I redirected the copy to the build-ios directory.

Comment thread src/app/CMakeLists.txt
"${rc_file}" PROPERTIES
OBJECT_DEPENDS ${win32_resources})
if (MINGW)
# CMake doesn't know how to process .rc files with MinGW.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why did you delete a bunch of comments that have nothing to do with macOS/iOS?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah these were accidental removals caused by some command line shenanigans. Forgot about that. I've fixed all the ones you pointed out. Thanks.

Comment thread src/app/CMakeLists.txt
BUNDLE DESTINATION "${CMAKE_INSTALL_APPDIR}")

if (Qt6_FOUND AND WIN32)
#manage plugins with windeployqt.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Again, deleted comment, unrelated to mac building.

Comment thread src/app/MainWindow.cpp Outdated
if (cmd.isEmpty()) {
QApplication::beep();
} else {
#if !defined(Q_OS_IOS)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: indentation

Comment thread src/app/MainWindow.cpp
Comment on lines +1432 to +1436
#if defined(Q_OS_IOS)
nullptr, QFileDialog::DontUseNativeDialog
#else
nullptr
#endif

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Source lines are over-indented

Comment thread CMakeLists.txt
Comment on lines -413 to -415
#with Qt6, winqtdeploy is used to deploy plugins; the method below does not work anymore.
if (NOT Qt6_FOUND)
# Install Qt plugins.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same.

Comment thread CMakeLists.txt
install_qt_plugin("imageformats" Qt::QSvgPlugin)
endif()

# Install MinGW runtime components.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Again.

Comment thread CMakeLists.txt
copy_to_build_dir(${ADDITIONAL_RUNTIME_LIBS_RELEASE} CONFIGURATIONS Release MinSizeRel RelWithDebInfo)
copy_to_build_dir(${ADDITIONAL_RUNTIME_LIBS_DEBUG} CONFIGURATIONS Debug)

# Generate the target that will actually do the copying.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here too.

Comment thread README.md
- Fakesigned IPAs installed with a free Apple ID via AltStore or Sideloadly expire after 7 days and need to be re-sideloaded. There is no such limitation when installed on a jailbroken device.
- The iOS file browser is rooted at the app's Documents folder. To make scans accessible, copy them to the ScanTailor Advanced folder via the Files app or Finder file sharing.
- Directory selection works fully with Magic Keyboard trackpad and keyboard navigation.
- To override the Qt path: `QT_IOS_DIR=~/Qt/6.x.x/ios QT_MACOS_DIR=~/Qt/6.x.x/macos ./build-ios.sh` No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

...Why is this last line waaay down here, instead of up where the build command is documented?

Comment thread src/core/CMakeLists.txt Outdated
Comment on lines +122 to +129
if(IOS)
# iOS: link only Qt modules; TIFF/PNG/JPEG/ZLIB are bundled inside Qt
if(Qt6_FOUND)
target_link_libraries(core
PUBLIC Qt::Core Qt::Gui Qt::Widgets Qt::Xml Qt::Network Qt::OpenGL Qt::Svg Qt::OpenGLWidgets
imageproc zones
fix_orientation page_split deskew select_content page_layout output)
endif()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This could be if(IOS AND Qt6_FOUND).

@somestupidgirl

Copy link
Copy Markdown
Author

As I mentioned in a couple of review comments, I have no role in this project. You should feel free to ignore any- or everything here, if you prefer, as none of my drive-by feedback carries any official weight whatsoever.

Just getting that out there right from the start.

All great feedback. Thank you. ❤️🙏

Comment thread src/app/CMakeLists.txt
add_executable(
${target_name} WIN32
${gui_only_sources} ${gui_only_ui_files}
${resource_files} ${win32_resource_file})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(Apologies, I thought this was included with my first review but it seems I didn't submit it.)

Fun Fact (actually it's kind of an obscure CMake detail): The original add_executable() — including the WIN32 and ${win32_resource_file} — worked cross-platform because CMake is smart enough to ignore things that have nothing to do with the platform being built. It's also smart enough to ignore replaced variables that contain the empty string.

So you can do away with all of this system detection, and just expand the original add_executable() from:

add_executable(
   ${target_name} WIN32 ${gui_only_sources} ${gui_only_ui_files}
    ${resource_files} ${win32_resource_file})

to:

add_executable(
   ${target_name} WIN32 MACOSX_BUNDLE
    ${gui_only_sources} ${gui_only_ui_files}
    ${resource_files} ${win32_resource_file})

And then change the definition of ${resource_files} so it's empty when building on iOS, instead, just like ${win32_resource_file} will be.

(You might want to flip it the other way around, and change ${resource_files} in the add_executable() to ${executable_resource_files}, instead. Then you can have an if(NOT IOS) that copies ${resource_files} into that variable. i.e.

set(resource_files
    resources.qrc
    dark_scheme/dark_scheme.qrc
    light_scheme/light_scheme.qrc)
list_items_prepend(resource_files "${SCANTAILOR_RESOURCES_DIR}/")

set(executable_resource_files "")
if(NOT IOS)
  set(executable_resource_files "${resource_files}")
endif()
# ...
add_executable(
   ${target_name} WIN32 MACOSX_BUNDLE
    ${gui_only_sources} ${gui_only_ui_files}
    ${executable_resource_files} ${win32_resource_file})

(That way, the original ${resource_files} is still available for use in other parts of the code. If it's needed. Maybe it isn't; I was thinking for install() use or the like, but you don't install compiled-in resources.)

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