View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0012154 | Scribus | Story Editor / Text Frames | public | 2014-03-22 15:48 | 2016-03-25 12:31 |
Reporter | FirasH | Assigned To | jghali | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Platform | x86_64 | OS | openSUSE | OS Version | 13.1 |
Product Version | 1.5.0svn | ||||
Target Version | 1.5.1 | Fixed in Version | 1.5.1svn | ||
Summary | 0012154: "Adjust Frame Height to Text" inconsistent behaviour with Middle/Bottom Vertical Alignment | ||||
Description | Adjust Frame Height to Text is inconsistent when Middle or Bottom Vertical Alignment is selected. | ||||
Steps To Reproduce | 0) Create a new document 1) Insert a Text Frame (higher is better) 2) Write one word 3) Properties > Text > Columns & Text Distances > Vertical Alignment > Middle or Bottom 4) Select the Text Frame > Right-click > Adjust Frame Height to Text The behaviour of Scribus is not useful for users who select "Adjust Frame Height to Text". Scribus should adjust the height as with "Top" selected or change both the height over and under the text so that the text keeps its position. | ||||
Additional Information | Scribus 1.5.0 SVN (18929) - Qt 5.2.1 | ||||
Tags | boxes, HOST-Oman | ||||
Patch | |||||
|
Agreed. Reproduced on r19232 (OS X 10.8.5 with Homebrew packaged Qt5) |
|
Hi The function m_textDistanceMargins.bottom() in the function setTextFrameHeight always returns zero. do you think that this is the problem? |
|
Please correct me if I am wrong.. I understand from the problem that we need a way to get the distance from the top of the text frame to the first line written on it . do you have idea how to do it. |
|
i did not have a look at the code, but i've now attached .sla where you can play around and see how the vertical alignment works. you should then open the properties palette at the "text > columns and distances" tab. you will see that you can define a top and bottom margin and still change the value for the vertical alignment. back to the auto-height feature. you should have a look at it and understand what values it changes when you trigger it. looking at the name of the method, i'm not sure that m_textDistanceMargins.bottom() is the function you're looking for. |
|
|
|
|
|
Thanks, now I am understand the different between the distance margin and vertical alignment. I searched on the code and I found that the function setTextFrameHeight is responsible of the option "Adjust Height to Text" so I modified it and I ended up with the attached patch. Please can you look at it. |
|
AdjustHeightToText.diff (2,545 bytes)
Index: scribus/pageitem_textframe.cpp =================================================================== --- scribus/pageitem_textframe.cpp (revision 20493) +++ scribus/pageitem_textframe.cpp (working copy) @@ -1654,6 +1654,7 @@ double scaleH = charStyle.scaleH() / 1000.0; double offset = hlcsize10 * (charStyle.baselineOffset() / 1000.0); style.setLineSpacing (calculateLineSpacing (style, this)); + setLSpace(calculateLineSpacing (style, this)); FlopBaseline = (current.startOfCol && firstLineOffset() == FLOPBaselineGrid); // find out about par gap and dropcap @@ -2995,6 +2996,8 @@ { regionMinY = static_cast<int>(qMax(0.0, floor(current.yPos - (asce + offset)))); regionMaxY = static_cast<int>(floor(current.yPos + desc)); + setMiniY( regionMinY); + setMaxiY( regionMaxY); } EndX = current.endOfLine(m_availableRegion, style.rightMargin(), regionMinY, regionMaxY); @@ -6085,7 +6088,9 @@ { //ugly hack increasing min frame`s haeight against strange glyph painting if it is too close of bottom double hackValue = 0.5; - + int miniY = getminiY(); + int maxiY = getmaxiY(); + maxY = (maxiY-miniY)*(textLayout.lines())+getLSpace()*(textLayout.lines() -textLayout.lines()/2 )/2; setHeight(ceil(maxY) + m_textDistanceMargins.bottom() + hackValue); updateClip(); invalid = true; @@ -6092,3 +6097,29 @@ m_Doc->changed(); m_Doc->regionsChanged()->update(QRect()); } +int PageItem_TextFrame::getminiY() +{ + return miniY; +} +int PageItem_TextFrame::getmaxiY() +{ + return maxiY; +} + +void PageItem_TextFrame::setMiniY(int value) +{ + miniY = ceil(value); +} +void PageItem_TextFrame::setMaxiY(int value) +{ + maxiY = ceil(value); +} +int PageItem_TextFrame::getLSpace() +{ + return lSpace; +} + +void PageItem_TextFrame::setLSpace(int value) +{ + lSpace = value; +} Index: scribus/pageitem_textframe.h =================================================================== --- scribus/pageitem_textframe.h (revision 20493) +++ scribus/pageitem_textframe.h (working copy) @@ -146,6 +146,9 @@ void invalidateNotesFrames(); private: + int miniY; + int maxiY; + int lSpace; NotesInFrameMap m_notesFramesMap; NotesInFrameMap updateNotesFrames(QMap<int, Mark*> noteMarksPosMap); //update notes frames content void updateNotesMarks(NotesInFrameMap notesMap); @@ -159,6 +162,12 @@ public: void setTextFrameHeight(); + int getminiY(); + int getmaxiY(); + int getLSpace(); + void setLSpace(int value); + void setMiniY(int value); + void setMaxiY(int value); }; #endif |
|
The idea of this patch is to use the function setHeight to determine the height of the text frame. The setHeight function works by receiving the sum of 3 variables : 1)The ceiling of variable called maxY which has the height need to be set. 2)The m_textDistanceMargins.bottom() variable which represent the distance between the margin and the text frame. 3)hackValue which was set as 0.5. So, I modified the maxY to be only the frame height. I used to do that the following formula: LH= height of line LN= number of lines LS= space between each two lines maxY = LH * LN + (LN-LN/2)/2 The height of each line I get it from subtract two variables(regionMinY and regionMaxY) |
|
Ibrahim : Is your patch OK aswell for bottom aligned text ? Have you tested the altered functions in other text layout situations ? |
|
I have a hard time understanding what the expected or desired behavior is. (and why) Perhaps Adjust Frame Height to Text should be disabled whenever Middle or Bottom is chosen for the position of the text. |
|
I think there is a desired behaviour : have the frame fit the text. It's possible to want this, whatever vertical align is set. AMOF as for now, "fit to text" brings the end of the frame to the bottom Y of the text. That is Ok for top aligned text, but not ok for center or bottom verticaly aligned text : - As for now, when vertical align is bottom, "fit to text" does nothing. - And when vertical align is center, "fit to text" makes the text height smaller, but not fit text height. Note to Ibrahim : For bottom aligned text, 'fit to text' should bring the *top* Y of the frame to the *top* of the text, and not change the bottom Y (of both text and frame). |
|
Here are 2 solutions, which do not involve and patches: 1. Click Undo BEFORE choosing Adjust Frame Height to Text 2. Select for text to align to Top instead of middle. THEN choose Adjust Frame Height to Text. Let's face it, when you choose Align to middle, then want to shrink the frame, YOU HAVE MADE A MISTAKE. IT'S NOT THE SOFTWARE'S FAULT YOU HAVE MADE A MISTAKE. |
|
Is this a bug? I put some text in a frame. I select Adjust Frame Height to Text. I add more text -- now the text overflows! Even if I select Adjust Frame Height to Text again, it still doesn't enlarge the frame! |
|
Greg : please create a new report for other issues. In comment to your http://bugs.scribus.net/view.php?id=12154#c37583 comment : You suggest to "undo". I guess when you suggest to undo, it is because your last action was to "vertical align to middle". This might be your case when testing this issue, but this is not the general case. In the general case, the user has "aligned to middle" (or to bottom) at some point in the DTP process, and later, maybe much later, the user sees that a better layout is to "adjust frame to height". It is not the software fault, and it is not a fault at all : it is a legit wish, part of a living design process that we can not discuss. The "adjust frame to height" exists. Its action should fit the clear intent that is expressed in its clear label : "adjust frame to height". It should deliver what it promises. And since it is possible to "adjust frame to height" without moving text, even when text is bottom or center aligned, it should not move the text. |
|
This is an unimportant bug. One of the most important features of Scribus to learn is how to Undo, once or serially. Undo helps us learn what not to do, and how our workflow is broken. My suggestion would be either of these: 1. Disabling Adjust Frame Height to Text when Align to Middle or Bottom is selected 2. Automatically switch to Align to Top when Adjust Frame Height to Text is selected |
|
Greg : Undo is not related to this issue and is not a solution to this issue. Greg : I understand your proposals about this issue, but that looks like a low profile solution. I think that fixing the issue with the original intent is better, all the more since its very close to be done properly. According to Ibrahim, his patch fixes the action when text is aligned to center. And fixing the action when text is aligned to bottom seems simpler, since what has is to be done is : - ensuring to not change the bottom Y of frame, so that the text doesnt move when the frame's height is changed. - change frame's height so as to bring the top Y of frame to the top Y of text beginning. AMOF, this action when text is bottom aligned is exactly symetrical to what it is when text is top aligned. (When text is top aligned, top Y is not changed, but bottom Y is brought to the bottom Y of the end of text). Ibrahim : Can you try to achieve this ? |
|
The patch doesn't seem to break anything. One thing to note is that you do not end up with the same result with Top, Middle, and Bottom alignment, as far as precisely where the line of text is in the frame. Is this what users will want and/or expect? Also (and maybe the same issue?), with Top alignment, the end result is slightly narrower than when using Middle or Bottom alignment. For example, with 12 pt text and Top, I get a frame height of 18.50 pt, while with Middle and Bottom alignment, I get 21.50 pt frame height. So it seems that maybe your calculations are still a bit off. As far as JLuc's contentions about trying to keep the text from moving on the page, I don't know if that's what users collectively want or not ... |
|
Regard to your comment gpittman http://bugs.scribus.net/view_user_page.php?id=708 1. I agree with Jloc that undo will not do any thing to solve this behaviour 2. The peace of code responsable of butting the text up , bottom and down is located in the function layout in pageitem_textframe.cpp . From the code you can recognize that it always up. |
|
ibrahim : i dont understand "From the code you can recognize that it always up." what does mean "it allways up" ? |
|
ok i understand what you mean with "it allways up" after testing your patch. So you have choosen to keep Y of frame, and as a consequence, the text moves. IMO It's not the smartest solution, but its better than what we have now. The result for bottom, center and top aligned text is not exactly the same, as Greg says, but that's probably because of how scribus deals with line's height, depending or not on the the glyphs in the text with high ascending or low descending parts. There are other reports about this. I'm afraid this is much more complex an issue and not tackled in this report. So i mark this patch as tested ok. |
|
I don't think we can mark this as fixed or resolved. It is considerably better, but this variable behavior depending on the vertical alignment will be confusing to users. It might be worth committing this just because it's so much better than previously, but perhaps this can be further tweaked. |
|
Here is somewhere else where the patch produces problems. I made a script some time ago to center text vertically in the frame. It does this by taking information about the lines of text, then adjusting the Top Distance. I could run this, then use Adjust Frame Height to Text, and the bottom of the frame would be just below the text. The patch ignores any Top Distance, and makes the frame too small - text overflows. To show this yourself, make a frame, put in a line of text, then adjust the Top Distance so that your text is about in the middle of the frame. Now select Adjust Frame Height to Text. Your frame will be too small to show the text. So this patch isn't looking so good at this point. |
|
I'm glad we agree on considering this patch as OK as is. As for the future improvement, i'm not sure, but it could be that the precise height of resized frame issue is not specifically related to "adjust frame height to text" but to a more general issue in scribus. See some reports that mention this type of issue : - 0011613 : Vertical position of line depending on text (same content, different layout) with cz patch provided, waiting to be tested and merged ! - 0012190 : Incorrect text align in textbox (final line). - 0012138 : Bottom align : text disappear when frame is too high |
|
12154_adjustFrameHeight_alignment.diff (2,537 bytes)
Index: scribus/pageitem_textframe.cpp =================================================================== --- scribus/pageitem_textframe.cpp (revision 20597) +++ scribus/pageitem_textframe.cpp (working copy) @@ -59,6 +59,7 @@ #include "ui/marksmanager.h" #include "undomanager.h" #include "undostate.h" +#include "units.h" #include "util.h" #include "util_math.h" @@ -6100,7 +6101,48 @@ //ugly hack increasing min frame`s haeight against strange glyph painting if it is too close of bottom double hackValue = 0.5; - setHeight(ceil(maxY) + m_textDistanceMargins.bottom() + hackValue); + if (textLayout.lines() <= 0) + return; + const LineSpec& firstLine = textLayout.line(0); + const LineSpec& lastLine = textLayout.line(textLayout.lines() -1); + + double y1 = firstLine.y - firstLine.ascent; + double y2 = lastLine.y + lastLine.descent; + double newHeight = ceil(y2) + m_textDistanceMargins.bottom() + hackValue; + if (verticalAlign != 0) + newHeight -= floor(y1); + + UndoTransaction undoTransaction; + if (UndoManager::undoEnabled() && (verticalAlign != 0)) + { + QString unitSuffix = unitGetStrFromIndex(m_Doc->unitIndex()); + int unitPrecision = unitGetPrecisionFromIndex(m_Doc->unitIndex()); + double unitRatio = m_Doc->unitRatio(); + QString owString = QString::number(oldWidth * unitRatio, 'f', unitPrecision) + " " + unitSuffix; + QString ohString = QString::number(oldHeight * unitRatio, 'f', unitPrecision) + " " + unitSuffix; + QString nwString = QString::number(m_width * unitRatio, 'f', unitPrecision) + " " + unitSuffix; + QString nhString = QString::number(m_height * unitRatio, 'f', unitPrecision) + " " + unitSuffix; + QString tooltip = QString(Um::ResizeFromTo).arg(owString).arg(ohString).arg(nwString).arg(nhString); + undoTransaction = undoManager->beginTransaction(Um::Selection, Um::ITextFrame, Um::Resize, tooltip, Um::IResize); + } + + if (verticalAlign == 1) + { + double newX = m_xPos + (m_height - newHeight) / 2 * cos(-m_rotation * M_PI / 180 - M_PI / 2); + double newY = m_yPos - (m_height - newHeight) / 2 * sin(-m_rotation * M_PI / 180 - M_PI / 2); + setXYPos(newX, newY); + } + else if (verticalAlign == 2) + { + double newX = m_xPos + (m_height - newHeight) * cos(-m_rotation * M_PI / 180 - M_PI / 2); + double newY = m_yPos - (m_height - newHeight) * sin(-m_rotation * M_PI / 180 - M_PI / 2); + setXYPos(newX, newY); + } + setHeight(newHeight); + + if (undoTransaction) + undoTransaction.commit(); + updateClip(); invalid = true; m_Doc->changed(); |
|
Not even looking at the patch correctness, this patch has a few cosmetic issues - it add public getters and setters for variables intended to stay private, so those getters and setters needs at the minimum to be private - private getters and setters, when they are so simple, are mostly useless and will impact (slightly) performance: calling a function is slower than performing variable assignment directly. On the correctness side, the patch will not work correctly if text frame contains several lines with different font size. I've uploaded a patch i did myself which attempts to fix this. I've also added a behavior change: the new patch attempt to keep text in place, ie if text is aligned to bottom, it will resize frame by moving the frame top. |
|
I did some more adjustments and committed. |
|
Tested ok. Precision seems to have increased along with smartness. Great ! |
|
Thanks jghali! That is exactly how it should be! Nice one! Fixed in 1.5.1.svn (20608). |
|
All is good now! |
|
Thanks to all who worked/commented on this bug Closing! |
Date Modified | Username | Field | Change |
---|---|---|---|
2014-03-22 15:48 | FirasH | New Issue | |
2014-06-15 13:06 | Kunda | Note Added: 0032174 | |
2014-07-06 16:06 | FirasH | Status | new => confirmed |
2014-08-21 11:56 | Kunda | Target Version | => 1.5.1 |
2015-09-17 20:08 | Kunda | Category | Story Editor / Text Frames => Story Ed/Txt Frames |
2015-09-17 20:12 | Kunda | Category | Story Ed/Txt Frames => Story Editor / Text Frames |
2015-11-03 05:20 | ibrahim | Tag Attached: HOST-Oman | |
2015-11-08 07:53 | ibrahim | Note Added: 0037311 | |
2015-11-08 10:43 | ibrahim | Note View State: 0037311: private | |
2015-11-08 10:51 | ibrahim | Note Added: 0037316 | |
2015-11-09 06:22 | ale | Note Added: 0037329 | |
2015-11-09 06:24 | ale | File Added: auto-height-and-margins.sla | |
2015-11-09 06:24 | ale | File Added: autoheight-and-margins.png | |
2015-11-10 06:28 | ibrahim | Note Added: 0037369 | |
2015-11-10 06:29 | ibrahim | File Added: AdjustHeightToText.diff | |
2015-11-10 08:08 | JLuc | Note View State: 0037311: public | |
2015-11-17 07:20 | ibrahim | Note Added: 0037522 | |
2015-11-21 18:50 | JLuc | Note Added: 0037576 | |
2015-11-21 21:25 | gpittman | Note Added: 0037578 | |
2015-11-21 22:12 | JLuc | Note Added: 0037579 | |
2015-11-21 22:13 | JLuc | Note Edited: 0037579 | |
2015-11-22 01:03 | gpittman | Note Added: 0037583 | |
2015-11-22 01:08 | gpittman | Note Added: 0037584 | |
2015-11-22 08:46 | JLuc | Note Added: 0037588 | |
2015-11-22 08:46 | JLuc | Note Edited: 0037588 | |
2015-11-22 14:34 | gpittman | Note Added: 0037589 | |
2015-11-22 22:24 | JLuc | Note Added: 0037592 | |
2015-11-23 02:46 | gpittman | Note Added: 0037595 | |
2015-11-23 07:37 | ibrahim | Note Added: 0037596 | |
2015-11-23 10:26 | JLuc | Note Added: 0037599 | |
2015-11-23 11:44 | JLuc | Note Added: 0037601 | |
2015-11-23 11:45 | JLuc | Tag Attached: patch_tested_ok | |
2015-11-23 13:57 | gpittman | Note Added: 0037607 | |
2015-11-24 00:19 | gpittman | Note Added: 0037621 | |
2015-11-24 09:13 | JLuc | Note Added: 0037636 | |
2015-11-24 09:13 | JLuc | Relationship added | related to 0011613 |
2015-11-24 09:14 | JLuc | Note Edited: 0037636 | |
2015-11-29 13:30 | jghali | File Added: 12154_adjustFrameHeight_alignment.diff | |
2015-11-29 13:43 | jghali | Note Added: 0037730 | |
2015-11-29 13:46 | jghali | Note Edited: 0037730 | |
2015-11-29 15:01 | jghali | Note Edited: 0037730 | |
2015-11-29 17:45 | Kunda | Tag Detached: patch_tested_ok | |
2015-11-29 17:45 | Kunda | Tag Attached: #please_test | |
2015-11-30 23:40 | jghali | Note Added: 0037757 | |
2015-11-30 23:40 | jghali | Status | confirmed => resolved |
2015-11-30 23:40 | jghali | Fixed in Version | => 1.5.1svn |
2015-11-30 23:40 | jghali | Resolution | open => fixed |
2015-11-30 23:40 | jghali | Assigned To | => jghali |
2015-12-01 10:04 | JLuc | Note Added: 0037770 | |
2015-12-01 16:55 | FirasH | Note Added: 0037775 | |
2015-12-02 02:01 | gpittman | Note Added: 0037782 | |
2015-12-02 11:47 | Kunda | Note Added: 0037787 | |
2015-12-02 11:47 | Kunda | Status | resolved => closed |
2016-01-24 12:17 | FirasH | Tag Detached: #please_test | |
2016-03-25 12:31 | Kunda | Tag Attached: boxes |