pixel-cookers / redmine-theme

A nice, dark and blue theme for Redmine.
http://pixel-cookers.github.io/redmine-theme/
MIT License
118 stars 58 forks source link

A first css patch for Redmine 2.1 #12

Closed GreenRover closed 11 years ago

GreenRover commented 11 years ago

Here is a first patch for Redmine 2.1

ic commented 11 years ago

@GreenRover: I have hit the same problem as you with the priorities on Redmine 2.1.2. However your patch is not really fixing the issue: Shifting the priority index will work as long as priorities are not customized in Redmine.

It turns out the problem is not (imho) in PixelCookers, but in Redmine... The classes assigned by Redmine lead to HTML like:

<tr id="issue-1234" class="hascontextmenu odd issue status-1 priority-4 ">

Where priority-4 is the priority with ID=4 in the enumerations table. But the class should actually be priority-2, which is the "position" of priority with ID=4. With that corrected, themes work properly. Note that this change corresponds to your fix, where priority values are all shift by 2 (just a coincidence due to the default database).

The enumerations table:

mysql> select * from enumerations;
+----+-------------------------+----------+------------+-------------------+--------+------------+-----------+
| id | name                    | position | is_default | type              | active | project_id | parent_id |
+----+-------------------------+----------+------------+-------------------+--------+------------+-----------+
|  1 | User documentation      |        1 |          0 | DocumentCategory  |      1 |       NULL |      NULL |
|  2 | Technical documentation |        2 |          0 | DocumentCategory  |      1 |       NULL |      NULL |
|  3 | Low                     |        1 |          0 | IssuePriority     |      1 |       NULL |      NULL |
|  4 | Normal                  |        2 |          1 | IssuePriority     |      1 |       NULL |      NULL |
|  5 | High                    |        3 |          0 | IssuePriority     |      1 |       NULL |      NULL |
|  6 | Critical                |        4 |          0 | IssuePriority     |      1 |       NULL |      NULL |
|  7 | Immediate               |        5 |          0 | IssuePriority     |      0 |       NULL |      NULL |
|  8 | Design                  |        1 |          0 | TimeEntryActivity |      1 |       NULL |      NULL |
|  9 | Development             |        2 |          0 | TimeEntryActivity |      1 |       NULL |      NULL |
| 10 | Operations              |        3 |          0 | TimeEntryActivity |      1 |       NULL |      NULL |
| 11 | Testing                 |        4 |          0 | TimeEntryActivity |      1 |       NULL |      NULL |
| 12 | Documentation           |        5 |          0 | TimeEntryActivity |      1 |       NULL |      NULL |
| 13 | Meeting                 |        6 |          0 | TimeEntryActivity |      1 |       NULL |      NULL |
| 14 | Code Review             |        7 |          0 | TimeEntryActivity |      1 |       NULL |      NULL |
+----+-------------------------+----------+------------+-------------------+--------+------------+-----------+
14 rows in set (0.00 sec)

Anyway, I will report this to the Redmine project.

GreenRover commented 11 years ago

Yes it is not a good fix, redmine should no use numbers, but the update from 2.0 to 2.1 should shift by 2 for every one or iam wrong?

ic commented 11 years ago

The shift by 2 is just a coincidence due to the default database contents. If a Redmine admin changes the priorities (see the 5 lines with type IssuePriority in the above extract) for new or shuffled records, the current code will break.

For example, if I create priority "Fatal", the ID in the table will probably be 15. And themes' code would break on "priority-15".

Looking at older Redmine 1.2.2 code, the "right" priority class for "normal" should be priority-2, not priority-4. The error happens seemingly as 2.1.2 uses priority's ID instead of priority's position.

Anyway your fix is a good workaround for anyone who needs a solution right away. But it is not a long-term solution, and I do not think it is a good idea to merge it into the PixelCookers codebase (just my opinion, I am hitting the problem same as you actually).

GreenRover commented 11 years ago

Yes its realy a quick and dirty fix.

Didt you allready tried to explain the issue to the redmine developers?

ic commented 11 years ago

I am digging the Redmine issue list to see if there is not already a ticket there. If not, I'll fill one with the contents exposed here.

Btw I tried 3 other themes, and they all break consistently. That suggests the problem is probably in Redmine...

ic commented 11 years ago

Update: A fix seems coming up in Redmine 2.2.0.

http://www.redmine.org/issues/12216

For the records, this is a known issue, and the Redmine team claims it is not a problem:

http://www.redmine.org/issues/11880

A few people disagree, including me now.

ic commented 11 years ago

Heiko, I have decided to use your patch for our internal deployment. Thank you for your work!

I now believe that it could be merged into PixelCookers. It is not a "proper" fix, but given that Redmine 2.1.x will NOT be fixed, that is a practical way to stay compatible. It will break only for the few people like me who use custom priorities---a known issue with a proper warning to user should do it.

And from Redmine 2.2.0, PixelCookers and all other themes will have to adapt to the new API anyway.

lmeyer commented 11 years ago

I am currently working on a better solution for redmine v2.2 which brings css classes not based on ID.

@GreenRover, you just have modified the css file but this theme uses less. The next less compilation would erase your work.

If you want to change the less for the redmine 2.1 version, tell me. I will create a 2.1 support branch and you will be able to redo your PR.

Cheers