Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

wrong icon theme is copied #131

Closed
hkollmann opened this issue Feb 8, 2018 · 16 comments
Closed

wrong icon theme is copied #131

hkollmann opened this issue Feb 8, 2018 · 16 comments

Comments

@hkollmann
Copy link
Member

If you create an application with qx create myapp -I -v

you get
qx.Theme.define("myapp.theme.Theme", { meta : { color : myapp.theme.Color, decoration : myapp.theme.Decoration, font : myapp.theme.Font, icon : qx.theme.icon.Oxygen, appearance : myapp.theme.Appearance } });

after qx compile you have the Tango theme in resources:

myapp\source-output\resource\qx\icon\Tango????

@hkollmann hkollmann changed the title wrong theme is copied wrong icon theme is copied Feb 8, 2018
@johnspackman
Copy link
Member

There are two issues here - firstly, qx create should specify an icon theme in the theme and also specify an environment variable qx.icontheme; at the moment it is inconsistent, and either should have icon: qx.theme.icon.Tango or add "environment": { "qx.icontheme": "Oxygen" } in compile.json (setting icon: qx.theme.icon.Tango would be best because that's the default in 5.x)

Secondly, even when this was done it would make little or no difference - the Qooxdoo framework explicitly refers to @asset(qx/icon/Tango/.... so Oxygen is effectively ignored. According to the docs, it is possible to change these @asset tags to something like @asset(qx/icon/${qx.icontheme}/... but this does not work in the generator, eg it fails with:

>>> Warning: Warning: (qx.theme.indigo.Appearance): Cannot replace macro 'qx.icontheme' in @asset hint
>>> Warning: No resource matched @asset(qx/icon/${qx.icontheme}/16/actions/dialog-cancel.png) (qx.theme.simple.Appearance)

Yet the files exist:

$ find source/resource -name dialog-cancel.png
source/resource/qx/icon/Oxygen/16/actions/dialog-cancel.png
source/resource/qx/icon/Tango/16/actions/dialog-cancel.png

The @asset(qx/icon/${qx.icontheme}/... does work in the compiler, and the changes to the framework are relatively minor so if the generator can be fixed the this problem can be fixed also.

@hkollmann
Copy link
Member Author

So to get Oxygen 2 places must be edited: the themes file and compile.json, right?

@cboulanger
Copy link
Contributor

see also qooxdoo/qooxdoo#9485

@johnspackman
Copy link
Member

@hkollmann yes plus the qooxdoo framework files, and then the generator has to be fixed so that it can accept variables in the @asset tags (which it's supposed to be doing already)

@cboulanger
Copy link
Contributor

cboulanger commented Feb 15, 2018

As I have written in qooxdoo/qooxdoo#9485, the problem wouldn't exist if the framework used aliases instead of hard-coded (or even partly aliased) paths. If I had time, I'd create a PR replacing all paths with aliases, but unfortunately, I don't...

@cboulanger
Copy link
Contributor

cboulanger commented Feb 15, 2018

In my view, icon paths shouldn't be in class files to begin with, it violates the DRY principle and makes changing icon sets impossible.

@hkollmann
Copy link
Member Author

With the generator you get the oxygen theme with mention it in the themes class. Tango theme is not copied in this case. So you can use oxygen icons in your own classes

@johnspackman
Copy link
Member

please can you check something for me - is it the case that (with the generator) if you use the Oxygen icon theme, the generator will not include the qx/icon/Tango/* files, even though they are specified by @asset directives, and instead choose the Oxygen equivalents?

@hkollmann
Copy link
Member Author

hkollmann commented Feb 15, 2018

Will do so on sunday - only ipad avaible yet
I am not sure if both are copied.

@hkollmann
Copy link
Member Author

hkollmann commented Feb 17, 2018

compiled with generaty.py

+---Oxygen
|   +---16
|   |   +---actions
|   |   +---apps
|   |   +---mimetypes
|   |   \---places
|   \---64
|       \---actions
\---Tango
    +---16
    |   +---actions
    |   +---apps
    |   +---mimetypes
    |   \---places
    +---22
    |   +---mimetypes
    |   \---places
    \---32
        +---mimetypes
        \---places

compiled with qx:

\---Tango
    +---16
    |   +---actions
    |   +---apps
    |   +---mimetypes
    |   \---places
    +---22
    |   +---mimetypes
    |   \---places
    +---32
    |   +---mimetypes
    |   \---places
    \---64
        \---actions

@hkollmann
Copy link
Member Author

adding

  "environment": { 
      "qx.icontheme": "Oxygen" 
  },     

does the trick. Both iconsets are copied.

One problem left: The icontheme must be metioned in two different places,

  • compile.json

  • Theme.js

Something like

qx.Theme.define("vzaweb.theme.Theme", {
  meta : {
    color : vzaweb.theme.Color,
    decoration : vzaweb.theme.Decoration,
    font : vzaweb.theme.Font,
    icon : "qx.theme.icon.${qx.icontheme}",
    appearance : vzaweb.theme.Appearance
  }
});

is not possible.

@hkollmann
Copy link
Member Author

Where do you get the warnings in the generator? If i changed @asset(qx/icon/Tango to @asset(qx/icon/${qx.icontheme} in the framework/source folder generate works fine for my project

@hkollmann
Copy link
Member Author

Found the warning. It's generated during creating the apiviewer.
Question is if we can change the framework and ignore the warning.

@johnspackman
Copy link
Member

aha, I just saw all the warnings and assumed it was the app generation.

I'm still confused though - I don't get how the generator knows to switch to different icon sets, because I can't find any reference to qx.icontheme in the JS code, and when it does switch to Oxygen that means that it should only copy Oxygen icons ...

@hkollmann
Copy link
Member Author

this will be done with qooxdoo/qooxdoo#9495

@hkollmann
Copy link
Member Author

Working and qooxdoo path is merged. Thanks @johnspackman !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants