-
Notifications
You must be signed in to change notification settings - Fork 8
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
Improve app structure #83
base: master
Are you sure you want to change the base?
Conversation
There are a lot of changes but most of them are due to empty translation file deletion |
Thanks! I'll try to look at this soon. I don't really have an opinion but the |
Not sure about moving the screenshots. The AppStore relies on them. So if we change the paths to the screenshots we'll break the screenshots in AppCenter until a new release is made and approved. And worse, for elementary v5 they will always be broken because I won't do another release for v5. |
) | ||
|
||
install_data( | ||
meson.project_name() + '.contract', | ||
install_dir: join_paths(get_option('datadir'), 'contractor') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the contract install logic but keep the contract file?
I know the contract file will no longer work: elementary/contractor#34. Maybe we should just remove all the contract logic in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I'll add it back so it can be removed in another PR
'com.github.peteruithoven.resizer', | ||
['vala', 'c'], | ||
meson_version : '>= 0.47', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not define the min. required meson?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely should be defined!
Meson can refuse to run on older versions, while printing an informative error message telling you that your version of Meson is too old. Additionally, it also prints informative warnings when you use features too new for the minimum required version to handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it back but even elementary apps does not include this. I assume developers should check that their development tools are up to date before trying to build the app. But I can add it back if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make life harder, when you could make life easier instead? :)
meson_version : '>= 0.47', | ||
license: 'GPL-3.0+' | ||
'c', 'vala', | ||
version: '1.0.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to limit the amount of places where the version is defined, to ideally nowhere but the tag. Is this really useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know but elementary does in their app as well as many other developers so.
print('Updating desktop database…') | ||
subprocess.call(['update-desktop-database', '-q', desktop_database_dir]) | ||
print('Updating icon cache…') | ||
subprocess.call(['gtk-update-icon-cache', '-qtf', path.join(datadir, 'icons', 'hicolor')]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this logic coming from? We're asking the desktop to update it's icon cache? Is that common practice? Does this make sense with it being a flatpak?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be replaced by https://mesonbuild.com/Gnome-module.html#gnomepost_install but that requires meson 0.57.0 as a minimum.
This is very common practice when installing normally, to /usr or /usr/local -- although flatpak may internally handle running this when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a common practice when building the app locally with meson. Even if your app is deployed as a Flatpak you should keep the meson install config.
Some of these comments are just questions, I'm not exactly an expert on Linux app development. I'm very spoiled in the web dev realm :P |
You're right! It is not that important but I think the most common one is LICENSE regarding elementary OS projects so I made the change 😄
I didn't think about that. Good point. I'll revert this I'll do the changes when I have some time |
I made a few improvements to your app structure. These are not breaking changes. Let me know if you have any questions 😄
Changelog:
COPYING
toLICENSE
meson.build
filesmeson.build
filespost_install.py
scriptI would be happy to update French translation once this will have been reviewed
I tried to build and run the app locally and I had no issues