wf49670 / ppgen

Post-processing generator for DP
6 stars 4 forks source link

Feature inline image #11

Closed davem2 closed 10 years ago

davem2 commented 10 years ago

For my current project I needed the ability to display illustrations side by side so I added this feature to ppgen. The changes consist of a new option for the .il align parameter 'i' (inline), and a new directive .ic (inline container). This is a big change; I'm sure you have thought about this problem more than I have and just wanted to suggest a possible solution. Consider this pull request as an opening for discussion.

Here is an example of how to display two images side by side:

.ic
.il id=i096a fn=i_096a.jpg w=30% alt='' align='i' cj='c'
.ca <i>Fig. 7.</i>
.il id=i096b fn=i_096b.jpg w=30% alt='' align='i' cj='c'
.ca <i>Fig. 8.</i>
.ic-

HTML result from above code:

<div class='inlinecontainer'>
  <div id='i096a'  class='figinline id009'>
    <img src='images/i_096a.jpg' alt='' class='ig009' />
    <div class='ic009'>
      <p><i>Fig. 7.</i></p>
    </div>
  </div>
  <div id='i096b'  class='figinline id010'>
    <img src='images/i_096b.jpg' alt='' class='ig010' />
    <div class='ic010'>
      <p><i>Fig. 8.</i></p>
    </div>
  </div>
</div>

CSS result from above code:

div.inlinecontainer { clear: both; margin: 0em auto; text-align: center; max-width: 100%; }
.figinline { display:inline-block; vertical-align:middle; max-width:100%; margin:1em; text-align: center;}
div.figinline p { text-align:center; text-indent:0; }

Originally I had an override in place to use display:block rather than display:inline-block for eBook readers

@media handheld { .figinline { display:block; }}

But found that inline-block was handled properly on all devices I tested against and the override was not needed.

It's possible in the future that the .ic directive could be used for inline display of other block elements besides img (like a multi column layout?)

windymilla commented 10 years ago

What I tried is grabbing David's code and running it in on a few examples. The behaviour is different to the trials I did, but that is because I was working with fixed image sizes, whereas ppgen now uses percentages for image sizes. Hence if you have two 30% illos, they will fit side by side, and never switch to being displayed vertically above one another. I think all that is then working as you want and as I would expect (with the change to display:block for handhelds).

What I got a bit confused over, (and there is probably a perfectly good explanation which involves the relative rate of creation and destruction of the neurons in my brain) is captions. I tried using some of the newer settings, like ew (for epub width) and cj (for caption justification). They didn't seem to work for me. I then tried using cj on an normal illo (not inside ic) and I was getting a "warning: unprocessed value in illustration: cj=l"

As you know, I don't actually use ppgen in anger (if that's the right phrase) so am probably doing something wrong, or possibly those edits aren't in David's fork/pull/? (pardon my github ignorance) or I didn't get what I thought I was getting.

Anyway, what I would like to check is how the various illo/caption options behave in relation to the .ic directive, so that any discrepancies can be fixed or at least documented.

Hope that makes sense Nigel

davem2 commented 10 years ago

I just spent a couple minutes playing around with the new ew parameter and found a bug. Looks like the ew related CSS is only generated if the .il statement uses align=c (center). I looked at the related code and the handling of cases where align != c is commented out (see below).

    if ia['align'] == 'c':
      my_width = int(re.sub("%", "", ia["ew"]))
      my_lmar = (100 - my_width) // 2
      my_rmar = 100 - my_width - my_lmar
      self.css.addcss("[610] @media handheld {{ .{} {{ margin-left:{}%; width:{}; }}}}".format(idn, my_lmar, ia["ew"]))
    # else:  # floated l or right
    #   self.css.addcss("[610] @media handheld {{ .{} {{ width:{}; }}}}".format(idn, ia["ew"]))

After reactivating that code, ew handling seems to work as expected (using normal .il statements with align=l/r and the new .ic stuff with align=i).

I tested all flavours of cj= against the new stuff and found no discrepancies.

I wasn't able to reproduce the "warning: unprocessed value in illustration: cj=l" problem. What was the full .il statement that caused this?

windymilla commented 10 years ago

Thanks David

I spotted what I had done wrong with cj, so that was a red herring.

I don't know about that commented-out code. It might be deliberate - Roger will know anyway.

Nigel

On 27 September 2014 17:47, davem2 notifications@github.com wrote:

I just spent a couple minutes playing around with the new ew parameter and found a bug. Looks like the ew related CSS is only generated if the .il statement uses align=c (center). I looked at the related code and the handling of cases where align != c is commented out (see below).

if ia['align'] == 'c':
  my_width = int(re.sub("%", "", ia["ew"]))
  my_lmar = (100 - my_width) // 2
  my_rmar = 100 - my_width - my_lmar
  self.css.addcss("[610] @media handheld {{ .{} {{ margin-left:{}%; width:{}; }}}}".format(idn, my_lmar, ia["ew"]))
# else:  # floated l or right
#   self.css.addcss("[610] @media handheld {{ .{} {{ width:{}; }}}}".format(idn, ia["ew"]))

After reactivating that code, ew handling seems to work as expected (using normal .il statements with align=l/r and the new .ic stuff with align=i).

I tested all flavours of cj= against the new stuff and found no discrepancies.

I wasn't able to reproduce the "warning: unprocessed value in illustration: cj=l" problem. What was the full .il statement that caused this?

— Reply to this email directly or view it on GitHub https://github.com/rfrank/ppgen/pull/11#issuecomment-57058424.

ghost commented 10 years ago

Nigel, David: has the .ic code been tested enough now? Nigel wrote "Anyway, what I would like to check is how the various illo/caption options behave in relation to the .ic directive, so that any discrepancies can be fixed or at least documented." Do I need to put together a test suite that combines .ic with all the caption options or do we believe we understand it enough now? I don't want to rush this onto the users if we aren't all comfortable with it. I can build and test combinations but that would have to wait until I get back to Denver and that's still a week away. Or maybe I'm not understanding the last few exchanges and perhaps the original pull request plus some additional changes are all we need and ready to go.

windymilla commented 10 years ago

There's just the question of the ew bug (or deliberate limitation) that David raised above. I don't think any of it needs to be released before you return to Denver though, unless David thinks otherwise.

ghost commented 10 years ago

"There's just the question of the ew bug (or deliberate limitation) that David raised above."

I can't figure out why it was commented out. I know there is code if it's centered to put margins left and right for epub that doesn't exists if it's left or right, but the doesn't mean I can leave out the width piece. I'm guessing it was being tested with width being applied in a different CSS line. Anyway I activated that commented-out code and tested it and it seems right. I also found an outright bug that's in the existing release regarding the ew parameter. I'll do a little more testing tonight and then I'm not sure of the github details to merge David's pull with my additional fixes. The only way I know how to do that is to merge his pull request and then immediately check it out and make my changes on top of that. There's probably a better way: I check out what's there now, make my two corrections, and then do the merge of his code with that. But I'm not sure that works. I'm not sure his can merge with something other than what he originally pulled and modified.

Anyway I think we are very close and I may have a chunk of time to finish testing this even before Denver. If I do, unless some other showstopper has come up, likely I'll publish this tonight.

windymilla commented 10 years ago

I would like to discuss the side-by-side options with advantages / disadvantages. That will hopefully guide us as to what should be the ppgen output, and indeed if ppgen should handle side-by-side illos at all, or if it's sufficiently obscure that it's reasonable to escape to literal HTML.

We have Dave's version (or slight variations thereon) which have an inline-block div that contains the illo and caption. This can be made to work with percentage or pixel widths. It also gives a fluid layout, i.e. illos flow to the next line with their caption if they don't fit on the current line. Of course that's only needed if you use pixel widths - percentages (if not too close to 100% total) will be a fairly fixed layout. The main disadvantage is that an illo with caption can be truncated and the caption (and part of the illo) lost, e.g. if viewing a fairly tall illo on a device in landscape mode.

We have an alternative of mine that doesn't group the illo with the caption, but has them all in separate inline blocks. Disadvantage: it can only work well with percentage widths. It doesn't really need the display:block override for handhelds however, since each thing is a separate entity.

Finally, we have Roger's table layout proposal. This has the advantage of working on all devices without truncation. It has the disadvantage of not providing a fluid layout for pixel-width illos. Also some people are allergic to tables if they perceive that the table is being used for non-table data.

Does everyone think that's a reasonable summary, or have I missed something?

Nigel

ghost commented 10 years ago

I think that's a reasonable summary. What jumped out at me is the mention of images in pixels. In ppgen, images specified in pixels are immediately turned into a percent (if I remember my coding) because of the widely varying and fixed screen resolutions on ereader hardware. If the image is going to percentages anyway, why do we have to consider any solution with respect to dimensions in pixels? By the time it gets to the layout code, it's always in percent. I feel like I'm missing something.

windymilla commented 10 years ago

I only mentioned images in pixels because David said he was setting them for the HTML width, something like w=600 ew=50%. I'm sure he can clarify that though.

wf49670 commented 10 years ago

Roger commented: "What jumped out at me is the mention of images in pixels. In ppgen, images specified in pixels are immediately turned into a percent (if I remember my coding) because of the widely varying and fixed screen resolutions on ereader hardware."

Image widths in px are left as px in the HTML output. They are converted to percentages only for epub output. The epub image width defaults to the HTML image width if not specified by the user (and if specified by the user is in %). Then ppgen checks for "px"in the epub image width, and if found it converts it to a percentage-based value.

wf49670 commented 10 years ago

Question: One reason that Nigel's version separates the captions from the images, if I remember correctly, is to allow proper alignment of the images when one of them has a longer caption. Is that still an issue with Dave's version? It's not one that Nigel mentioned in the summary above so I'm not sure.

davem2 commented 10 years ago

As Walt pointed out, we still need to be concerned about dimensions expressed in pixels for the HTML versions. This change is not just for ereaders.

Some additions to Nigel's summary. I'm going to refer the different implementations as v1(inline-block, image+caption), v2(inline-block, seperate img caption), and v3(table version).

v1

v2

v3 Haven't seen this implementation yet

@rfrank: It would be helpful if we had a development branch in place, this would allow us to collaborate/experiment in source code and see these things in action without affecting the end user release. It would also get rid of the roadblock this pull request has created in development.

We talked a little about this before as a solution to the version string bump issue, and there was concern that it would make it harder for users to download the latest version. Adding this branch would not affect the way ppgen is distributed. Master branch would continue to be the latest release and end users could grab a .zip of that. I would be happy to walk you through the changes, (it's not as bad as it sounds)

ghost commented 10 years ago

Absolutely yes let's make a development branch. Limited experience with GIT has been a big piece of my reluctance. If you can walk me through it, then let's make it happen. Maybe 5 minutes on Skype is all we need? I need to know not only how to set it up but how to manage it. I also need a simple explanation I can put in the forum thread or in a project README.txt that makes it really easy for the users to get the right production download.

windymilla commented 10 years ago

I don't think my v2 is worth pursuing. It doesn't have the fluidity benefit nor the robustness of v1, though it does have the desired alignment. I think v3 provides everything except the fluidity

@davem2 How strongly do you feel about the fluid layout for ebooks? My personal view is that it's very important for general web design of course, but much less so for ebooks. The original paper book didn't have it, so we aren't losing original features. If there was an easy solution that retained it without any downsides, it would be a good addition, but I don't feel it's an important enough feature to lose alignment, and either risk truncation or to force all illos one above the other (from inline-block override).

I had a thought that maybe v3 could be done by expanding on the existing table syntax, but the more you think about it, the less suitable that seems.

davem2 commented 10 years ago

@windymilla I see the fluid layout as a nice to have but not essential. I like how it handles pretty much anything you throw at it using a simple implementation.

If caption alignment is a must have (for the two real cases I needed this it wasn't), I'm thinking a table implementation, possibly limited to one row of images/captions might be best (for multirow just use two consecutive .ic blocks). I'm not familiar with any quirks with tables on ereaders though, we might be trading one set of issues for another. I have a gut feeling this might be messy implementing.

How is this situation currently handled by PPers? Why not emulate that? Someone must have come up with a good solution to this already.. When I first implemented this it was to handle this situation. I took the feedback I recieved/found in the forums (break up into seperate figures, use inline-block for side to side) and built my implementation around that.

@rfrank Not sure about Skype (I'm really bad at communicating verbally, and just communicating in general for that matter), but I will write up a walk-through detailing the setup and workflow (there really is not very much to it). Are you still using the GUI tool or have you gone to using the command line? I'm not familiar with the GUI tool so would only be able to offer command line instructions.

As far as the download instructions, the steps would not change from how they were explained on the ppgen forum. Users would come here and click the Download Zip button or grab a copy on your website.

Kind of off topic, but, I've been using git for my PP projects and would suggest you try it out. It's a nice way to get comfortable using git and have found the history/diffs/multi-version backups is very useful. It would also be a great learning tool (for technical types like me at least) to see the commit history and diffs of an experienced PPer, (similar to the example checklists on the DP wiki but with all the detailed changes at the same time). I may put up a public repo of my stuff in the future for others to learn from.

windymilla commented 10 years ago

The problem with "emulating what PPers currently do" is that there are conflicting views on what is important to retain/gain through different methods, e.g. faithful copy of original, robustness through epubmaker conversion to handheld versions, size of illo restrictions, (un)desirability of thumbnail/fullsize implementation, etc. So people do different things, and there isn't an obvious "right answer". Also, it is necessary to remember that the four of us here have significantly above average knowledge of HTML/CSS and epub limitations, compared with a typical PPer.

It would be interesting to see how git & PP work together. As you say, it could be interesting for some PPers to see. I suppose it would depend on whether the detail that was specific to one book obscured the process, or whether there would be lessons that could be applied to other books. Certainly worth publicising at least one.

wf49670 commented 10 years ago

I would also be interested in seeing how Git works for PPing, and what advantages it might have. I have just started playing with it a little bit for that purpose, and so far I'm not sure I understand what the benefits would be, or what the best workflow would be. At this point I suspect that most PPers would find it to be another tool they'd have to install and learn to use, and without some clearly obvious advantages over simply saving multiple copies as the work proceeds I'm not sure how many would choose to use it..

@davem2: Perhaps you could describe your workflow for us? As it's not really related to the current discussion, it might be better to open a new Issue here against ppgen, and we can have a discussion there.

ghost commented 10 years ago

I'm going to close this pull request because I'm trying (very hard) to set up Git with a "develop" branch. The goal is to be able to have several versions of ppgen available at the same time as branches off develop. We can try different ways for illustrations, for example, and then I would merge the one we select back into develop and then develop into master.

davem2 commented 10 years ago

I've moved the "Using git for PPing thread" over to here:

https://github.com/rfrank/ppgen/issues/15