Bug 8624 - famfamfam should be outside theme directory
Summary: famfamfam should be outside theme directory
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low minor (vote)
Assignee: Jared Camins-Esakov
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks: 8662 8793
  Show dependency treegraph
 
Reported: 2012-08-13 00:01 UTC by Jared Camins-Esakov
Modified: 2013-12-05 20:05 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8624: Move famfamfam out of theme directory (522.79 KB, patch)
2012-08-13 00:50 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8624: Move famfamfam out of theme directory (522.84 KB, patch)
2012-08-21 19:02 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-08-13 00:01:50 UTC
The famfamfam image set is a standard third-party collection that is not "skinned" for different themes. Therefore, it should be moved outside the theme directory to /koha-tmpl/opac-tmpl/famfamfam/
Comment 1 Jared Camins-Esakov 2012-08-13 00:24:44 UTC
Excuse me. famfamfam should be in koha-tmpl/opac-tmpl/lib/famfamfam/
Comment 2 Jared Camins-Esakov 2012-08-13 00:50:32 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2012-08-13 10:06:44 UTC
This seems like a good opportunity to move famfamfam into the images directory, unless someone knows a reason why it isn't there already? Same goes for itemtypeimg too I guess.
Comment 4 Jared Camins-Esakov 2012-08-13 10:58:56 UTC
Well, there is an argument that the other images might be theme specific, I suppose. However I see no reason why we couldn't pull all images out of the themes, and require theme writers to use theme-specific images when they want to.
Comment 5 Owen Leonard 2012-08-21 19:02:51 UTC
Created attachment 11726 [details] [review]
[SIGNED-OFF] Bug 8624: Move famfamfam out of theme directory

The patch moves the famfamfam image set out of the
koha-tmpl/opac-tmpl/prog/famfamfam into koha-tmpl/opac-tmpl/famfamfam

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Jonathan Druart 2012-08-22 08:43:14 UTC
QA Comments:

Seems good but there are 2 occurrences not replaced in INSTALL.opensuse:
    INSTALL.opensuse:cp koha-tmpl/opac-tmpl/prog/famfamfam/silk/error_go.png blib/OPAC_TMPL_DIR/prog/famfamfam/silk/error_go.png
    INSTALL.opensuse:cp koha-tmpl/opac-tmpl/prog/famfamfam/mini/note.gif blib/OPAC_TMPL_DIR/prog/famfamfam/mini/note.gif

And a question: It is not possible to move the koha-tmpl/intranet-tmpl/prog/img/famfamfam/ directory into the same one? And then move them to koha-tmpl/images/famfamfam directly?
Comment 7 Jared Camins-Esakov 2012-08-22 11:31:49 UTC
(In reply to comment #6)
> QA Comments:
> 
> Seems good but there are 2 occurrences not replaced in INSTALL.opensuse:
>     INSTALL.opensuse:cp koha-tmpl/opac-tmpl/prog/famfamfam/silk/error_go.png
> blib/OPAC_TMPL_DIR/prog/famfamfam/silk/error_go.png
>     INSTALL.opensuse:cp koha-tmpl/opac-tmpl/prog/famfamfam/mini/note.gif
> blib/OPAC_TMPL_DIR/prog/famfamfam/mini/note.gif

I will do a follow-up.

> And a question: It is not possible to move the
> koha-tmpl/intranet-tmpl/prog/img/famfamfam/ directory into the same one? And
> then move them to koha-tmpl/images/famfamfam directly?

Now that you suggest it, I think it might be possible. If Owen can't think of any reason not to, I will submit a revised patch to do this.
Comment 8 Owen Leonard 2012-08-22 12:36:26 UTC
(In reply to comment #7)
> Now that you suggest it, I think it might be possible. If Owen can't think
> of any reason not to, I will submit a revised patch to do this.

I see no reason not to from the template management point of view. If it is feasible, let's do it. And the same with itemtypeimg?
Comment 9 Jared Camins-Esakov 2012-08-22 12:49:42 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > Now that you suggest it, I think it might be possible. If Owen can't think
> > of any reason not to, I will submit a revised patch to do this.
> 
> I see no reason not to from the template management point of view. If it is
> feasible, let's do it. And the same with itemtypeimg?

Actually, this would not work with standard installs.
Comment 10 Mason James 2012-09-14 15:23:15 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Now that you suggest it, I think it might be possible. If Owen can't think
> > > of any reason not to, I will submit a revised patch to do this.
> > 
> > I see no reason not to from the template management point of view. If it is
> > feasible, let's do it. And the same with itemtypeimg?
> 
> Actually, this would not work with standard installs.

hmm, unless we had +symlinks enabled in apache, yeah?
Comment 11 Jared Camins-Esakov 2012-09-14 15:26:51 UTC
> > > > Now that you suggest it, I think it might be possible. If Owen can't think
> > > > of any reason not to, I will submit a revised patch to do this.
> > > 
> > > I see no reason not to from the template management point of view. If it is
> > > feasible, let's do it. And the same with itemtypeimg?
> > 
> > Actually, this would not work with standard installs.
> 
> hmm, unless we had +symlinks enabled in apache, yeah?

Ooh, that's an intriguing idea. I'm not sure how to do that in a way that won't break dev installs, though, since most developers don't update their koha-httpd.conf files.
Comment 12 Mason James 2012-09-14 15:27:54 UTC
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>

patch looks GREAT!!!, passing QA… ;)

$ koha-qa.pl 

testing 1 commit(s) (applied to commit 0acebb8)
 * 052bd3d Bug 8624: Move famfamfam out of theme directory
      koha-tmpl/opac-tmpl/lib/famfamfam/AR.png
...
      koha-tmpl/opac-tmpl/lib/famfamfam/BK.png
      koha-tmpl/opac-tmpl/prog/famfamfam/silk/zoom_in.png
      koha-tmpl/opac-tmpl/prog/famfamfam/silk/zoom_out.png

* koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt                      OK
* koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACDetail.xsl               OK
* koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl              OK
* koha-tmpl/opac-tmpl/prog/en/xslt/NORMARCslim2OPACDetail.xsl              OK
* koha-tmpl/opac-tmpl/prog/en/xslt/NORMARCslim2OPACResults.xsl             OK
Comment 13 Paul Poulain 2012-09-17 16:52:39 UTC
(In reply to comment #11)
> > > > > Now that you suggest it, I think it might be possible. If Owen can't think
> > > > > of any reason not to, I will submit a revised patch to do this.
> > > > 
> > > > I see no reason not to from the template management point of view. If it is
> > > > feasible, let's do it. And the same with itemtypeimg?
> > > 
> > > Actually, this would not work with standard installs.
> > 
> > hmm, unless we had +symlinks enabled in apache, yeah?
> 
> Ooh, that's an intriguing idea. I'm not sure how to do that in a way that
> won't break dev installs, though, since most developers don't update their
> koha-httpd.conf files.

mmm... I'm not sure I should push this patch. Is there still a debate ? why famfam and not itemtypeimg ? Has anyone checked it will it work with standard installs as well as dev ones ?
Comment 14 wajasu 2012-09-17 19:23:17 UTC
ok, i did a standard install from the signed off patch.  i inspected /usr/share/koha/.../lib/famfamfam and such and it looked fine.
i turned on the XSLT Details( with "default" and see the BK.png in my details in opac and staff client.  I'm assuming the SIGNED-OFF  patch had other places confirmed.

wajasu
Comment 15 Paul Poulain 2012-09-18 10:11:12 UTC
I find some references to prog/famfam using:
grep -R "prog/famfamfam" *|more

not a big deal =
INSTALL.opensuse:cp koha-tmpl/opac-tmpl/prog/famfamfam/silk/error_go.png blib/OPAC_TMPL_DIR/prog/famfamfam/silk/error_go.png
INSTALL.opensuse:cp koha-tmpl/opac-tmpl/prog/famfamfam/mini/note.gif blib/OPAC_TMPL_DIR/prog/famfamfam/mini/note.gif

much more important:
koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetDetail.xsl:        <xsl:element name="img"><xsl:attribute name="src"
>/opac-tmpl/prog/famfamfam/<xsl:value-of select="$materialTypeCode"/>.png</xsl:attribute><xsl:attribute name="alt"></xsl:attr
ibute></xsl:element>
koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:                <xsl:when test="$typeOf008='Mon'"><img s
rc="/opac-tmpl/prog/famfamfam/silk/book.png" alt="Bok" title="Bok"/> Bok</xsl:when>
koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:                <xsl:when test="$typeOf008='Per'"><img s
rc="/opac-tmpl/prog/famfamfam/silk/newspaper.png" alt="Periodika" title="Periodika"/> Periodika</xsl:when>
koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:                <xsl:when test="$typeOf008='Fil'"><img s
rc="/opac-tmpl/prog/famfamfam/silk/computer_link.png" alt="Fil" title="Fil"/> Fil</xsl:when>
koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:                <xsl:when test="$typeOf008='Kar'"><img s
rc="/opac-tmpl/prog/famfamfam/silk/map.png" alt="Kart" title="Kart"/> Kart</xsl:when>
koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:                <xsl:when test="$typeOf008='FV'"><img sr
c="/opac-tmpl/prog/famfamfam/silk/film.png" alt="Film og video" title="Film og video"/> Film og video</xsl:when>
koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:                <xsl:when test="$typeOf008='Mus'"><img s
rc="/opac-tmpl/prog/famfamfam/silk/sound.png" alt="Musikktrykk og lydopptak" title="Musikktrykk og lydopptak"/> Musikk</xsl:w
hen>

Please also fix them to gain "patch pushed" ;-)
Comment 16 Jared Camins-Esakov 2012-09-18 10:16:50 UTC
(In reply to comment #15)
> I find some references to prog/famfam using:
> grep -R "prog/famfamfam" *|more
> 
> not a big deal =
> INSTALL.opensuse:cp koha-tmpl/opac-tmpl/prog/famfamfam/silk/error_go.png
> blib/OPAC_TMPL_DIR/prog/famfamfam/silk/error_go.png
> INSTALL.opensuse:cp koha-tmpl/opac-tmpl/prog/famfamfam/mini/note.gif
> blib/OPAC_TMPL_DIR/prog/famfamfam/mini/note.gif
> 
> much more important:
> koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetDetail.xsl:       
> <xsl:element name="img"><xsl:attribute name="src"
> >/opac-tmpl/prog/famfamfam/<xsl:value-of select="$materialTypeCode"/>.png</xsl:attribute><xsl:attribute name="alt"></xsl:attr
> ibute></xsl:element>
> koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:       
> <xsl:when test="$typeOf008='Mon'"><img s
> rc="/opac-tmpl/prog/famfamfam/silk/book.png" alt="Bok" title="Bok"/>
> Bok</xsl:when>
> koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:       
> <xsl:when test="$typeOf008='Per'"><img s
> rc="/opac-tmpl/prog/famfamfam/silk/newspaper.png" alt="Periodika"
> title="Periodika"/> Periodika</xsl:when>
> koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:       
> <xsl:when test="$typeOf008='Fil'"><img s
> rc="/opac-tmpl/prog/famfamfam/silk/computer_link.png" alt="Fil"
> title="Fil"/> Fil</xsl:when>
> koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:       
> <xsl:when test="$typeOf008='Kar'"><img s
> rc="/opac-tmpl/prog/famfamfam/silk/map.png" alt="Kart" title="Kart"/>
> Kart</xsl:when>
> koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:       
> <xsl:when test="$typeOf008='FV'"><img sr
> c="/opac-tmpl/prog/famfamfam/silk/film.png" alt="Film og video" title="Film
> og video"/> Film og video</xsl:when>
> koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:       
> <xsl:when test="$typeOf008='Mus'"><img s
> rc="/opac-tmpl/prog/famfamfam/silk/sound.png" alt="Musikktrykk og lydopptak"
> title="Musikktrykk og lydopptak"/> Musikk</xsl:w
> hen>
> 
> Please also fix them to gain "patch pushed" ;-)

This patch is OPAC-only.
Comment 17 Paul Poulain 2012-09-19 12:21:28 UTC
(In reply to comment #16)
> This patch is OPAC-only.

Right, but, even if we speak of intranet files, they contain opac-tmpl links:

> koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl:       
> <xsl:when test="$typeOf008='Mon'"><img s
> rc="/opac-tmpl/prog/famfamfam/silk/book.png" alt="Bok" title="Bok"/>
> Bok</xsl:when>

=> /opac-tmpl/prog/famfamfam/silk/book.png

link that is now wrong

Adding Magnus to this bug !
(and back to failed QA)
Comment 18 Magnus Enger 2012-09-19 13:28:17 UTC
Wow, I messed up nicely! But I think the fix for this should be a separate bug, so I have created Bug 8793 - "Remove references to OPAC icons from NORMARC staff XSLT" and will have a fix for it shortly.
Comment 19 Paul Poulain 2012-09-19 13:40:35 UTC
OK, this one is passing QA then, i'll push it now
Comment 20 Paul Poulain 2012-09-19 13:41:40 UTC
Patch pushed to master

(suggesting keeping it for 3.10 in case we've missed something)
Comment 21 Chris Cormack 2013-04-20 22:58:15 UTC
Released in 3.10.0