[Feature]: Relative app icon paths #62

neutrino2211 · 2022-06-19T14:24:34Z

Move app icons into $app/Resources folder
Add asset manager class in libfoundation
Add higher-priority "icon_rel_path" field to application info.json
Add assets field for folders to be copied into the build app dir
Programatically add apps to DockViewController
Use AssetManager to fetch app icon path in AppDelegates

- Move app icons into $app/Resources folder - Add asset manager class on libfoundation - Add higher-priority "icon_rel_path" field to application info.json - Add assets field for folders to be copied into the build app dir - Programatically add apps to DockViewController - Use AssetManager to fetch app icon path in AppDelegates

neutrino2211 · 2022-06-19T14:26:14Z

For issue #61

nimelehin

Thanks for working on this!

I have left several comments and please format all files. We use webkit code style. As I see you use vscode, you can consider downloading a plugin and configuring it with webkit code style.

Overall what you have done is a really great start point. Add we might considering the same asset management as used in iOS or similar (https://developer.apple.com/documentation/foundation/bundle). We might create shared Bundle class, which is inited at the start up of an application with info from a info.json file linked with the application. Finally we could use something like UI::Bundle::main.path("README.txt") inside our app to the the absolute path of a file from an asset folder.

If you have any questions feel free to ask them.

libs/libfoundation/include/libfoundation/AssetManager.h

userland/system/dock/DockViewController.h

nimelehin · 2022-06-19T18:37:14Z

userland/system/applist/AppListViewController.h

@@ -45,6 +45,12 @@ class AppListViewController : public UI::ViewController<AppListView> {
        LG::PNG::PNGLoader loader;

        std::string icon_path = jdict_root->data()["icon_path"]->cast_to<LFoundation::Json::StringObject>()->data();
+        std::string icon_rel_path = jdict_root->data()["icon_rel_path"]->cast_to<LFoundation::Json::StringObject>()->data();
+
+        if (!icon_rel_path.empty()) {


Consider adding an "empty" icon if icon_rel_path is not set. (here they are: https://github.com/opuntiaOS-Project/opuntiaOS/tree/master/base/res/icons/apps/missing.icon)

Okay, so that means icon_path generated by prepare_app.py should be completely omitted? Or is there any plan for it? Maybe always setting it to the missing icon path

We should unify icon_path and icon_rel_path for sure, so yeah we should omit one of them. But also in case if there is no such field in info.json which describes location of an icon, we should set a "missing icon".

As an alternative to this icon_path and icon_rel_path we could provide path to asset folder in the info.json. And to require have a "special" folder which contains icons inside the asset folder. In other words to have a provided path to an asset folder and look there for a "AppIcon" folder to get all of the icons.

neutrino2211 · 2022-06-25T06:34:08Z

I currently have both the assets/AppIcon and icon_rel_path present in my latest commit but will remove icon_rel_path if you see the AppIcon implementation as good enough.

Also, I noticed that it was hard to convince the build system that things have changed if I didn't edit a cpp file, which got me thinking...

Would it be weird to have the configurations inside info.json to move into the BUILD.gn file? Sort of as a single file for all configuration needs.

nimelehin · 2022-06-25T14:45:30Z

Also, I noticed that it was hard to convince the build system that things have changed if I didn't edit a cpp file, which got me thinking...

Yeah, currently build system knows nothing about info.json files. To let it know we should generate correct inputs with a configuration file in action("prepare_$app_name") of template("opuntiaOS_application").

Would it be weird to have the configurations inside info.json to move into the BUILD.gn file? Sort of as a single file for all configuration needs

I think fixing a build system should be enough.

nimelehin · 2022-06-25T14:54:50Z

You can squash 6d12fe6 Adhere to webkit style guidelines commit into the previous one.

We also use a commit naming like [module]: short description, like [Feature]: Relative app icon paths -> [userland] Relative app icon paths.

I will take a deeper look a bit later.

nimelehin · 2022-06-30T17:21:00Z

Hey, sorry for a long reply.

I got a crash during loading of AppList on x86 GNU testing your PR, have you seen this before?

Assertion failed: ObjectT::ObjType == m_type, function assert_object, file ../libs/libfoundation/include/libfoundation/json/Object.h:47
EAX: 0x25  EBX: 0x8  ECX: 0x6
EDX: 0x0  ESI: 0x0  EDI: 0x0
EIP: 0x804c694  ESP: 0xbfffedb8  EBP: 0xbfffedc8

[1] 0x804c694 : kill
[2] 0x804c6c1 : raise
[3] 0x804b59a : abort
[4] 0x804825a : __init_app_delegate
[5] 0x8049020 : _ZN18DockViewController13view_did_loadEv
[6] 0x804e2b8 : _ZN11LFoundation9EventLoop4pumpEv
[7] 0x804e3c0 : _ZN11LFoundation9EventLoop3runEv
[8] 0x804815a : main
[9] 0x804b20a : _start

neutrino2211 · 2022-07-03T17:47:08Z

I haven't honestly, that's new. I apologise for my silence as well. The past week was extremely busy for me.
I'll do a fresh clone and try compiling, I might have missed something

neutrino2211 · 2022-07-03T18:07:47Z

I also have the same issue, all this time I've been building for aarch64 because I'm using an M1Pro. I'll take a look at it

nimelehin

Thanks homescreen a lot cleaner now :)

icon_path also looks good, but maybe we could write path of assets folder to the config file and at runtime look AppIcon folder inside provided assets folder. What do you think?

nimelehin · 2022-07-08T18:41:15Z

libs/libfoundation/include/libfoundation/AssetManager.h

+class AssetManager {
+public:
+    AssetManager(const std::string& name)
+        :m_app_root(std::string() + "/Applications/" + name + ".app/Content") 


It would be better if we could look for Content folder getting the binary path. I would to implement this similar to Apple's Bundle. They have the similar to what you have implemented. bundle.main could be initialised with a binary path (see ProcessInfo::parse_info_file) during the startup of UI Apps (again as a ProcessInfo).

Both UI and Foundation libs I try to make as close as possible to Apple's versions (as I have plans on launching recompiled iOS apps one day). So it would be great that all APIs have the similar behaviour to Apple's.

nimelehin · 2022-07-08T18:43:02Z

userland/applications/activity_monitor/AppDelegate.cpp


 class AppDelegate : public UI::AppDelegate {
 public:
    AppDelegate() = default;
    virtual ~AppDelegate() = default;

    LG::Size preferred_desktop_window_size() const override { return LG::Size(220, 210); }
-    const char* icon_path() const override { return "/res/icons/apps/activity_monitor.icon"; }
+    const char* icon_path() const override 


While it's fine to keep for now, but it would be good to get rid of these icon_path() functions somehow. I didn't think on how to implement this, may be you can think of something.

neutrino2211 · 2022-12-29T16:42:24Z

Hey, I'm very sorry for the months of silence on this. A lot of things happened in my life these last few months that made open source impossible for me to do. Is the main branch still in need of this? If it is, I'd like to look into the ProcessInfo idea

neutrino2211 changed the title ~~[Feature]: Relative app icon paths #61~~ [Feature]: Relative app icon paths Jun 19, 2022

nimelehin reviewed Jun 19, 2022

View reviewed changes

neutrino2211 added 2 commits June 25, 2022 06:35

Adhere to webkit style guidelines

6d12fe6

Move to assets/AppIcon system

1322969

Use new icon system in mobile homescreen

d8ca13f

nimelehin reviewed Jul 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature]: Relative app icon paths #62

[Feature]: Relative app icon paths #62

neutrino2211 commented Jun 19, 2022

neutrino2211 commented Jun 19, 2022

nimelehin left a comment •

edited

Loading

nimelehin Jun 19, 2022

neutrino2211 Jun 20, 2022

nimelehin Jun 20, 2022

neutrino2211 commented Jun 25, 2022

nimelehin commented Jun 25, 2022

nimelehin commented Jun 25, 2022

nimelehin commented Jun 30, 2022

neutrino2211 commented Jul 3, 2022 •

edited

Loading

neutrino2211 commented Jul 3, 2022

nimelehin left a comment

nimelehin Jul 8, 2022

nimelehin Jul 8, 2022

neutrino2211 commented Dec 29, 2022

[Feature]: Relative app icon paths #62

Are you sure you want to change the base?

[Feature]: Relative app icon paths #62

Conversation

neutrino2211 commented Jun 19, 2022

neutrino2211 commented Jun 19, 2022

nimelehin left a comment • edited Loading

Choose a reason for hiding this comment

nimelehin Jun 19, 2022

Choose a reason for hiding this comment

neutrino2211 Jun 20, 2022

Choose a reason for hiding this comment

nimelehin Jun 20, 2022

Choose a reason for hiding this comment

neutrino2211 commented Jun 25, 2022

nimelehin commented Jun 25, 2022

nimelehin commented Jun 25, 2022

nimelehin commented Jun 30, 2022

neutrino2211 commented Jul 3, 2022 • edited Loading

neutrino2211 commented Jul 3, 2022

nimelehin left a comment

Choose a reason for hiding this comment

nimelehin Jul 8, 2022

Choose a reason for hiding this comment

nimelehin Jul 8, 2022

Choose a reason for hiding this comment

neutrino2211 commented Dec 29, 2022

nimelehin left a comment •

edited

Loading

neutrino2211 commented Jul 3, 2022 •

edited

Loading