View Issue Details

IDProjectCategoryView StatusLast Update
0012154ScribusStory Editor / Text Framespublic2016-03-25 12:31
ReporterFirasH Assigned Tojghali  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Platformx86_64OSopenSUSEOS Version13.1
Product Version1.5.0svn 
Target Version1.5.1Fixed in Version1.5.1svn 
Summary0012154: "Adjust Frame Height to Text" inconsistent behaviour with Middle/Bottom Vertical Alignment
DescriptionAdjust Frame Height to Text is inconsistent when Middle or Bottom Vertical Alignment is selected.
Steps To Reproduce0) 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 InformationScribus 1.5.0 SVN (18929) - Qt 5.2.1
Tagsboxes, HOST-Oman
Patch

Relationships

related to 0011613 newcbradney [PATCH] vertical position of line depending on text (same content, different layout) 

Activities

Kunda

2014-06-15 13:06

updater   ~0032174

Agreed. Reproduced on r19232 (OS X 10.8.5 with Homebrew packaged Qt5)

ibrahim

2015-11-08 07:53

reporter   ~0037311

Hi
The function m_textDistanceMargins.bottom() in the function setTextFrameHeight always returns zero. do you think that this is the problem?

ibrahim

2015-11-08 10:51

reporter   ~0037316

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.

ale

2015-11-09 06:22

manager   ~0037329

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.

ale

2015-11-09 06:24

manager  

ale

2015-11-09 06:24

manager  

autoheight-and-margins.png (33,256 bytes)   
autoheight-and-margins.png (33,256 bytes)   

ibrahim

2015-11-10 06:28

reporter   ~0037369

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.

ibrahim

2015-11-10 06:29

reporter  

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
AdjustHeightToText.diff (2,545 bytes)   

ibrahim

2015-11-17 07:20

reporter   ~0037522

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)

JLuc

2015-11-21 18:50

developer   ~0037576

Ibrahim :
Is your patch OK aswell for bottom aligned text ?
Have you tested the altered functions in other text layout situations ?

gpittman

2015-11-21 21:25

developer   ~0037578

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.

JLuc

2015-11-21 22:12

developer   ~0037579

Last edited: 2015-11-21 22:13

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).

gpittman

2015-11-22 01:03

developer   ~0037583

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.

gpittman

2015-11-22 01:08

developer   ~0037584

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!

JLuc

2015-11-22 08:46

developer   ~0037588

Last edited: 2015-11-22 08:46

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.

gpittman

2015-11-22 14:34

developer   ~0037589

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

JLuc

2015-11-22 22:24

developer   ~0037592

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 ?

gpittman

2015-11-23 02:46

developer   ~0037595

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 ...

ibrahim

2015-11-23 07:37

reporter   ~0037596

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.

JLuc

2015-11-23 10:26

developer   ~0037599

ibrahim : i dont understand "From the code you can recognize that it always up." what does mean "it allways up" ?

JLuc

2015-11-23 11:44

developer   ~0037601

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.

gpittman

2015-11-23 13:57

developer   ~0037607

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.

gpittman

2015-11-24 00:19

developer   ~0037621

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.

JLuc

2015-11-24 09:13

developer   ~0037636

Last edited: 2015-11-24 09:14

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

jghali

2015-11-29 13:30

administrator  

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();

jghali

2015-11-29 13:43

administrator   ~0037730

Last edited: 2015-11-29 15:01

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.

jghali

2015-11-30 23:40

administrator   ~0037757

I did some more adjustments and committed.

JLuc

2015-12-01 10:04

developer   ~0037770

Tested ok.
Precision seems to have increased along with smartness.
Great !

FirasH

2015-12-01 16:55

reporter   ~0037775

Thanks jghali!
That is exactly how it should be! Nice one!
Fixed in 1.5.1.svn (20608).

gpittman

2015-12-02 02:01

developer   ~0037782

All is good now!

Kunda

2015-12-02 11:47

updater   ~0037787

Thanks to all who worked/commented on this bug
Closing!

Issue History

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