Page MenuHomePhabricator

Elementary 1.16 breaks older Elm Theme Toolbar Icons
Closed, InvalidPublic

Description

ePad and ePhoto are both missing icons on their tool bar here with the 1.16 bindings VS 1.15.1 with my Radiance theme.

Screenshot:

http://i.imgur.com/wmuiJYB.png

Exact same ePad code, the one on the right is using elementary 1.15.1 the one on the left is using 1.16

Theme source -> https://github.com/JeffHoogland/MokshaRadiance

jeffhoogland updated the task description. (Show Details)
jeffhoogland raised the priority of this task from to Incoming Queue.
jeffhoogland added a project: Restricted Project.
jeffhoogland added subscribers: jeffhoogland, simotek.
zmike closed this task as Invalid.Nov 18 2015, 11:12 AM
zmike claimed this task.
zmike added a subscriber: zmike.

100% a broken theme on your end. Tested against the elm darkness theme in git, which I wrote and have not touched in years, and it works perfectly.

I don't know why/how it could have worked with < 1.16 other than to say that there was an elm bug which allowed it at the time and has since been fixed.

jeffhoogland changed the task status from Invalid to Wontfix.Nov 18 2015, 11:17 AM

So much for the theme API being stable.

This is why no one wants to spend time creating themes. In addition to being complex they don't stay working.

If you really want to go down the road, fine: I'll drive.

Here's your statement from IRC today which I took to be the reasoning why your theme should continue to work no matter what as long as there are no API changes:

<Jef91> zmike, it was based on the default theme to start
<Jef91> and edited from there

Let's look at the current state of your toolbar theme, as found here:

program { name: "st3";
   signal: "elm,icon,hidden"; source: "elm";
   script {
     new m = get_int(btmode);
      m |= ICON; set_int(btmode, m);
      eval_mode(m);
   }
}
program { name: "st4";
   signal: "elm,icon,visible"; source: "elm";
   script {
      new m = get_int(btmode);
      m &= ~ICON; set_int(btmode, m);
      eval_mode(m);
   }
}

Here's the same code from the current default toolbar theme:

program { name: "st3";
   signal: "elm,icon,hidden"; source: "elm";
   script {
     new m = get_int(btmode);
      m &= ~ICON; set_int(btmode, m);
      eval_mode(m);
   }
}
program { name: "st4";
   signal: "elm,icon,visible"; source: "elm";
   script {
      new m = get_int(btmode);
      m |= ICON; set_int(btmode, m);
      eval_mode(m);
   }
}

By comparing the two, a reader can see that there's a noticeable difference--namely that your theme adds the ICON state in the hide signal and removes it in the show signal, while the default theme does it the opposite way. This makes sense based on the way the theme operates in the embryo functions since label/icon visibility and state are determined by the presence of these flags such that (slightly simplified for ease of reading) having the ICON flag means the icon will be shown and not having it means that it will be hidden.

Now if you'll look at rELM740cc0751807bb0c0bbb393d6e0d2e1588eb0678, these two lines are part of an "undocumented change" which fixes an unrelated issue. But what you'll see is that the default theme also had these lines (not surprising, based on your concession that your theme is copied from the default and then modified from there). Using git blame to track down the original lines of code reveals that the default theme, in fact, had a bug from the start, and that toolbar icons were only visible here due to coincidence--that is to say, elementary was not enforcing icon visibility as it should have been, an issue which was resolved with the aforementioned commit.

Fixing a bug in the default theme is not indicative of an unstable API, and neither is enforcing behavior which is part of a stable API.

In the future I recommend that if you are going to copy code from upstream, you should consider also tracking and copying any changes to that code in order to avoid similar issues.

zmike changed the visibility from "All Users" to "Public (No Login Required)".Nov 18 2015, 11:39 AM

Cheers for the detailed description and analysis @zmike, I was going to ask a similar question, I did notice this change when I rebased all my themes against the latest the other week. I have no problems with themes breaking if its fixing bugs. Anyway cheers for the explanation

simotek changed the task status from Wontfix to Invalid.Nov 18 2015, 1:21 PM
raster added a subscriber: raster.Nov 18 2015, 4:00 PM

well tracked mike. i was JUST hunting myself... and said similarly - his theme hides when told to show...

and that was a bug in the original theme i fixed.

indeed if you copy a theme and then do your own - track changes to the original in case they are bug fixes.

that's the cost of forking ANYTHING. themes included.

I think it is difficult for people who makes themes to track changes in the code.
In this situation, it might be nice to have some announcement, like a warning for people who makes themes... no?

zmike added a comment.Nov 18 2015, 4:49 PM

I think it is difficult for people who makes themes to track changes in the code.
In this situation, it might be nice to have some announcement, like a warning for people who makes themes... no?

It's quite easy to create a herald rule in phabricator to notify you when any theme files have changed.

yeah - a herald rule if u care.. or simply a commit ti an edc file in the elm tree... :)

I just thought this could be on the release announcement, like an unavoidable abi/api change because of a bug. But this kind of bug might be hard to notice.

I read release announcements properly and would pay special attention to stuff like that cos its my job :-)