Add MacOS support #1045
Add MacOS support #1045
Conversation
Endle
commented
Jan 31, 2024
Endle
commented
- Load libintl.8.dylib on mac
- Check GTK schema
1) Load libintl.8.dylib on mac 2) Check GTK schema
|
It does not start on Windows because of |
|
Thank you. I don’t know the mechanism behind it. How about changing from raising an exception into printing a warning?
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: kbengs ***@***.***>
Sent: Friday, February 2, 2024 12:21:47 PM
To: pdfarranger/pdfarranger ***@***.***>
Cc: Zhenbo Li ***@***.***>; Author ***@***.***>
Subject: Re: [pdfarranger/pdfarranger] Add MacOS support (PR #1045)
It does not start on Windows because of check_gtk_schema_exists() . Is that needed on Linux and Windows?
—
Reply to this email directly, view it on GitHub<#1045 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAYSQEIFMBX26YC26LDYZDLYRUOCXAVCNFSM6AAAAABCSHFMPGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRUGMZTKOBRG4>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Yes I think so and run the check only on "Darwin", if it is a potential problem there? |
pdfarranger/pdfarranger.py
Outdated
| def get_libintl_path(): | ||
| f = 'libintl.so.8' | ||
| if os.name == 'nt': | ||
| f = 'libintl-8' | ||
| if platform.system() == 'Darwin': | ||
| f = 'libintl.8.dylib' | ||
| return f |
There was a problem hiding this comment.
Can we get rid of the variable f here and return directly? Reorder the statements if overwriting was part of the logic but I guess that's not necessary here.
Also add an empty line after the function for readability.
pdfarranger/pdfarranger.py
Outdated
| def check_gtk_schema_exists(): | ||
| schemas = subprocess.run(["gsettings", "list-recursively"], | ||
| stdout=subprocess.PIPE, stderr=subprocess.PIPE, | ||
| check=True, | ||
| text=True) | ||
| return 'org.gtk.Settings.ColorChooser' in schemas.stdout | ||
| if not check_gtk_schema_exists(): | ||
| raise Exception('Found no schema files. You may need to set GSETTINGS_SCHEMA_DIR.') |
There was a problem hiding this comment.
I totally understand that you added this to help/guide users with the same issue. However we don't want it to break stuff with tests that would be fine without therefore we should either:
- Run it only on darwin if it is a common enough problem here
- Make it a warning only. On windows where it is not supposed to work ever we still shouldn't run it because spamming warnings that are unfixable and not helpful are still a bad idea imo.
Feel free to do a combination of both.
|
Thank you @dreua I totally agree with your comments. I'm sorry that I'm a bit distracted in this week. I'll modify my code and add a short note in the README when I have time. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| check=True, | ||
| text=True) | ||
| return 'org.gtk.Settings.ColorChooser' in schemas.stdout | ||
| except: |
Check notice
Code scanning / CodeQL
Except block handles 'BaseException' Note
There was a problem hiding this comment.
Let's specify this exception, I think a FileNotFoundError would be fine.
| check=True, | ||
| text=True) | ||
| return 'org.gtk.Settings.ColorChooser' in schemas.stdout | ||
| except: |
There was a problem hiding this comment.
Let's specify this exception, I think a FileNotFoundError would be fine.
| @@ -118,6 +126,23 @@ | |||
| from .config import Config | |||
| from .core import Sides, _img_to_pdf | |||
|
|
|||
| def check_gtk_schema_exists(): | |||
| # On Windows the GTK library has a different logic, so we're skipping the test | |||
| # See https://github.com/pdfarranger/pdfarranger/pull/1045/files#r1485570912 | |||
There was a problem hiding this comment.
Another trialing whitespace, just remove it. Many IDEs or editors have a setting to automatically strip or at least show it.
|
Could you rebase the branch on current main? Ideally also getting rid of the merge commits? Jerome recently changed something in the tests maybe that already fixes the failed test here. |
|
I still think this should be darwin only, .ie. There is not much reason to run it on Linux. It will slow down upstart of application (by 37ms). |
