fix: undo slide behavior for text component (fixed #315 #310) #314

jinwoo-kim-nhn · 2020-01-29T09:18:43Z

Please check if the PR fulfills these requirements

It's submitted to right branch according to our branching model
It's right issue type on title
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
It does not introduce a breaking change or has description for the breaking change

issue

Does not work when running undo #315
Improve the value not to be reflected in undoStack while dragging in the slider UI. #310
text 기능의 addText, changeTextStyle 명령에 대한 undo 또는 redo 실행시 상태가 되돌려지지 않음

문제점 파악 및 수정

1. 슬라이더 UI의 drag도중에는 undo 스택에 반영 안되도록 개선

rotate 기능에 구현된 부분을 참고하여 변경 (isSilent 플래그를 추가하여 운영)
fontSize가 변경될때는 마지막 스택의 텍스트 스타일 값을 항상 알고 있도록 변경하고, 그 값을 기준으로 동작 하도록 변경

2. redo 실행시 이미 저장되어 있는 undoData가 새로운 잘못된 값으로 변질 되면서 재차 undo, redo 를 실행하면 정확한 동작을 보장하지 않는 버그 수정

redo 실행시 이미 undoData가 있으면 값이 변경되지 않도록 수정

3. 텍스트 객체를 선택하면 UI에 반영이 안되는 현상 개선

ui/text.js에 setTextStyleStateOnAction 을 구현하여 요소가 선택되면 UI에 반영되도록 개선

4. underline 변경은 되돌려지지가 않는 현상 개선

페브릭 버전 2.0 부터 textDecoration이 제거되고 underline 속성을 사용하도록 되었기 때문에 연관된 부분 수정 - Textbox Text Decoration Issue fabricjs/fabric.js#3956

5. undo, redo 실행시 텍스트 객체가 선택되어 있으면 텍스트 객체가 변하면서 UI상태도 변해야 하지만 변하지 않는 버그 수정

undo, redo 실행시 deactivateAll() 을 실행 함으로서 버그처럼 보이던 원인을 제거 (선택된 객체의 변경만 UI에 반영되면 되므로)

…mage-editor into feat/undoSlideBehavior

junghwan-park · 2020-02-03T07:18:13Z

src/js/command/addText.js

+ return new Promise((resolve, reject) => {
+ if (!graphics.contains(this.undoData.object)) {
+ graphics.add(this.undoData.object);
+ resolve(this.undoData.object);


this.undoData.object는 캐싱할 수 있겠네요. 그리고 this.undoData가 없는 상황의 optional 처리는 필요하지 않을까요?

interface/command.js 의 생성자에서 this.undoData = {} 를 만들어 주고 있어서 딱히 optional 처리를 안해줘도 될것같습니다.

this.undoData.object 는 캐싱하도록 하겠습니다. 감사합니다

junghwan-park · 2020-02-03T07:18:52Z

src/js/command/addText.js

 return textComp.add(text, options).then(objectProps => {
- this.undoData.object = graphics.getObject(objectProps.id);
+ const textObject = graphics.getObject(objectProps.id);


objectProps.id는 그냥 디스트럭쳐링 해도 될 것 같아요

넵 반영하겠습니다. 감사합니다.

junghwan-park · 2020-02-03T07:19:51Z

src/js/command/changeTextStyle.js

+ targetObj.lastfontSizeUndoStack = targetObj[key];
+ undoData.styles[key] = undoValue;
+ } else {
+ undoData.styles[key] = undoValue;


이 코드는 두 분기 모두에서 실행되니, else 문은 필요 없어보입니다.

그렇네요. 감사합니다. 반영하겠습니다.

junghwan-park · 2020-02-03T07:23:21Z

src/js/command/changeTextStyle.js

 const textComp = graphics.getComponent(TEXT);
 const targetObj = graphics.getObject(id);
+ const isRedo = Object.keys(this.undoData).length;


Object.keys()를 사용해야 하나요? 특정 프로퍼티가 있는지를 확인하는게 더 안전하지 않을까 생각됩니다.

makeUndoData()내부를 보니 object, styles 프로퍼티가 있는지만 보면 되겠네요.

isRedo는 command.isRedo 함수로 빼서 Command의 기능으로 공통화 시키면 좋게습니다.

넵 그렇네요. shape쪽 pr에 같은 리뷰를 달아 주셔서 그쪽에서 일괄 처리해보겠습니다.

junghwan-park · 2020-02-03T07:41:57Z

src/js/action.js

+ fontFamily: 'Noto Sans',
+ fontStyle: this.ui.text.fontStyle,
+ fontWeight: this.ui.text.fontWeight,
+ underline: this.ui.text.underline


this.ui.text를 캐싱할 수 있을 것 같습니다.

제 의견은, destructuring하고 shorthand property를 쓰면 코드가 좀 더 깔끔해 질 것 같아요.

이부분은 동식 책임님 의견으로 고치겠습니다.

dongsik-yoo

[02/04] 리뷰 완료했습니다. 수고하셨습니다.

dongsik-yoo · 2020-02-04T02:13:00Z

src/js/action.js

@@ -264,9 +266,9 @@ export default {
 */
 _textAction() {
 return extend({
- changeTextStyle: styleObj => {
+ changeTextStyle: (styleObj, isSilent) => {


파라미터 이름이 isSilent인 것이 잘 와닿지 않습니다. undo 스택에 쌓지 않는다는 의미를 담고 있지 못한 것 같아요. undoable과 같은 이름은 어떨까요?

이 부분은 undoable이 더 명확할것 같네요. 다른 작업의 브랜치들에서 같은 이름으로 작업중이어서 이번 pr에 고치지 않고 마지막 filter쪽 연관 이슈가 작업된 pr에서 일괄적으로 고치겠습니다.

dongsik-yoo · 2020-02-04T02:14:47Z

src/js/action.js

+ fontFamily: 'Noto Sans',
+ fontStyle: this.ui.text.fontStyle,
+ fontWeight: this.ui.text.fontWeight,
+ underline: this.ui.text.underline


제 의견은, destructuring하고 shorthand property를 쓰면 코드가 좀 더 깔끔해 질 것 같아요.

dongsik-yoo · 2020-02-04T02:20:16Z

src/js/command/addText.js

 return textComp.add(text, options).then(objectProps => {
- this.undoData.object = graphics.getObject(objectProps.id);
+ const textObject = graphics.getObject(objectProps.id);
+ textObject.lastfontSizeUndoStack = textObject.fontSize;


각 오브젝트나 클래스에 last~~ 를 추가하는 패턴이 계속 추가될 것 같은데요, 각 객체마다 값을 들고 있지 않고, undo 쪽에 구조를 만들어서 undo에 필요한 데이터를 들고 있는 것은 어떨까요?

텍스트 쪽은 여러개의 텍스트 객체가 생길 수 있고 텍스트 객체마다 다른 상태의 last~~ 값을 가져야 하므로 객체마다 값을 들고 있게 하였습니다.

this.undoData에서 관리하는것은 command의 인스턴스가 들고 있으므로 데이터를 필요할때 commandStack을 전부 뒤져서 써야 하므로 적합하지 않은것 같고.

다른 구조를 만들더라도 텍스트 객체가 추가되거나 삭제될때마다 텍스트객체 아이디 별로 값을 관리해 줘야 하는 관리하기 복잡한 코드가 생길것 같습니다. 현재로서는 target 객체가 last 값을 알고 있는게 가장 효율적이라고 생각이 듭니다. 작업하면서 해당 부분이 거슬리긴 했지만... 더 좋은 방법이 생각나기 전까지는 이 방법이 가장 효율적일것 같습니다.

이 부분은 구두로 논의한 방식으로 커멘드의 undoData 를 활용하는 방식으로 수정하겠습니다. 다른 pr에서도 다 같은 방식으로 사용중이어서 이슈를만들어 한꺼번에 처리하겠습니다.

dongsik-yoo · 2020-02-04T02:21:03Z

src/js/command/changeTextStyle.js

+function makeUndoData(styles, targetObj, isSilent) {
+ const undoData = {};
+
+ undoData.object = targetObj;


이 두 줄은 변수 선언할 때 같이 작성하면 코드가 줄겠네요.

넵 이 부분 저도 고치려고 생각만 하고 있다가. 아직까지 못고쳤네요.. 반영하겠습니다.감사합니다.

dongsik-yoo · 2020-02-04T02:24:25Z

src/js/command/changeTextStyle.js

 const textComp = graphics.getComponent(TEXT);
 const targetObj = graphics.getObject(id);
+ const isRedo = Object.keys(this.undoData).length;


isRedo는 command.isRedo 함수로 빼서 Command의 기능으로 공통화 시키면 좋게습니다.

dongsik-yoo · 2020-02-04T02:28:14Z

src/js/component/text.js

+ * @param {string} textDecoration - text decoration option string
+ * @returns {object} adapt object for override
+ */
+ _getTextDecorationAdaptObject(textDecoration) {


질문입니다. textDecoration가 문자열이라 이 함수가 리턴하는 객체의 프러퍼티는 1개만 true일거 같은데요, 그러면 3가지 속성을 동시에 설정할 수는 없는 거예요?

예전 fabricjs버전에서는 안되었었기 때문에 현재 imageEditor에서도 하나씩 뿐이 선택 안되도록 되어있고, 업데이트된 fabricjs버전에서는 3개 동시에 적용 가능하지만, 저희 옵션도 수정해야되서 옵션 인터페이스 수정이 필요합니다. 이 이슈의 작업과는 직접적인 관계가 없으므로 새로운 이슈를 만들어서 추후에 개선하겠습니다.

히스토리가 있었군요. 더 멋진 이미지 에디터를 위해 애써 주셔서 감사합니다.

dongsik-yoo · 2020-02-04T02:29:16Z

src/js/imageEditor.js

- changeTextStyle(id, styleObj) {
- return this.execute(commands.CHANGE_TEXT_STYLE, id, styleObj);
+ changeTextStyle(id, styleObj, isSilent) {
+ const executeMethodName = isSilent ? 'executeSilent' : 'execute';


silent는 네이밍이 좀 안 맞는거 같아요 ㅠ

넵 이 부분은 다음 이슈에서 일괄 수정해보겠습니다.

dongsik-yoo · 2020-02-04T02:32:33Z

src/js/ui/text.js

+
+ setAlignState(value) {
+ const button = this._els.textAlignButton;
+ button.classList.remove(this.align);


우리가 IE9+ 이상 지원이라 classList를 쓸 수가 없을 것 같은데요, 확인 부탁 드립니다.

코드에 폴리필을 'src/js/polyfill.js' 넣어서 사용중입니다.

…hn#314) * middle commit * feat: apply input ui in range component * feat: appy value with change input * feat: some refactoring (change naming, added jsdoc) * fix: added jsdoc for missing jsdoc * feat: some refactoring for remove event binding * fix: rotation setAngle undo bug * feat: improve undo slide behavior for text component - middle commit * fix: undo redo bug for text menu - prototype commit * feat: added test for text change undo redo * middle commit * feat: apply input ui in range component * feat: appy value with change input * feat: some refactoring (change naming, added jsdoc) * feat: some refactoring for remove event binding * feat: improve undo slide behavior for text component - middle commit * fix: undo redo bug for text menu - prototype commit * feat: added test for text change undo redo * fix: incorrected rebase line delete * fix: unnessary text style property (textDecoration) * fixed: some refactoring * fix: fixed incorrected text option * fix: apply codereview Co-authored-by: superlucky84 <[email protected]>

jinwoo-kim-nhn added 10 commits January 20, 2020 15:29

middle commit

c4d6f70

feat: apply input ui in range component

147da8e

feat: appy value with change input

870caa8

feat: some refactoring (change naming, added jsdoc)

b212730

fix: added jsdoc for missing jsdoc

f3e4884

feat: some refactoring for remove event binding

890cf6e

fix: rotation setAngle undo bug

e9d56a7

feat: improve undo slide behavior for text component - middle commit

5c75833

fix: undo redo bug for text menu - prototype commit

8230949

feat: added test for text change undo redo

1c29e82

jinwoo-kim-nhn changed the title ~~Feat/undo slide behavior~~ feat undo slide behavior for text component Jan 30, 2020

jinwoo-kim-nhn changed the title ~~feat undo slide behavior for text component~~ fix: undo slide behavior for text component Jan 30, 2020

jinwoo-kim-nhn changed the title ~~fix: undo slide behavior for text component~~ fix: undo slide behavior for text component (fixed #315) Jan 30, 2020

jinwoo-kim-nhn added 10 commits January 30, 2020 11:53

middle commit

6d5ce77

feat: apply input ui in range component

cabfc40

feat: appy value with change input

f442167

feat: some refactoring (change naming, added jsdoc)

53e36e5

feat: some refactoring for remove event binding

2b55997

feat: improve undo slide behavior for text component - middle commit

8c2a67f

fix: undo redo bug for text menu - prototype commit

6bacd1c

feat: added test for text change undo redo

6650299

fix: incorrected rebase line delete

c62e33e

Merge branch 'feat/undoSlideBehavior' of https://github.com/nhn/tui.i…

fc31844

…mage-editor into feat/undoSlideBehavior

jinwoo-kim-nhn changed the base branch from fix/incorrectedUndoAngleForUndoData to master January 30, 2020 03:22

jinwoo-kim-nhn added 2 commits January 30, 2020 12:32

fix: unnessary text style property (textDecoration)

d42275c

fixed: some refactoring

004ae09

jinwoo-kim-nhn changed the title ~~fix: undo slide behavior for text component (fixed #315)~~ fix: undo slide behavior for text component (fixed #315 #310) Jan 30, 2020

fix: fixed incorrected text option

8c6b3fb

junghwan-park reviewed Feb 3, 2020

View reviewed changes

dongsik-yoo reviewed Feb 4, 2020

View reviewed changes

fix: apply codereview

825ee35

jinwoo-kim-nhn merged commit 73c528d into master Feb 4, 2020

jinwoo-kim-nhn deleted the feat/undoSlideBehavior branch March 20, 2020 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: undo slide behavior for text component (fixed #315 #310) #314

fix: undo slide behavior for text component (fixed #315 #310) #314

jinwoo-kim-nhn commented Jan 29, 2020 •

edited

Loading

junghwan-park Feb 3, 2020

jinwoo-kim-nhn Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020

junghwan-park Feb 3, 2020

jinwoo-kim-nhn Feb 4, 2020

junghwan-park Feb 3, 2020

jinwoo-kim-nhn Feb 4, 2020

junghwan-park Feb 3, 2020

dongsik-yoo Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

junghwan-park Feb 3, 2020

dongsik-yoo Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020

dongsik-yoo left a comment

dongsik-yoo Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

dongsik-yoo Feb 4, 2020

dongsik-yoo Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

dongsik-yoo Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

dongsik-yoo Feb 4, 2020

dongsik-yoo Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020

dongsik-yoo Feb 4, 2020

dongsik-yoo Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020

dongsik-yoo Feb 4, 2020

jinwoo-kim-nhn Feb 4, 2020

fix: undo slide behavior for text component (fixed #315 #310) #314

fix: undo slide behavior for text component (fixed #315 #310) #314

Conversation

jinwoo-kim-nhn commented Jan 29, 2020 • edited Loading

Please check if the PR fulfills these requirements

issue

문제점 파악 및 수정

1. 슬라이더 UI의 drag도중에는 undo 스택에 반영 안되도록 개선

2. redo 실행시 이미 저장되어 있는 undoData가 새로운 잘못된 값으로 변질 되면서 재차 undo, redo 를 실행하면 정확한 동작을 보장하지 않는 버그 수정

3. 텍스트 객체를 선택하면 UI에 반영이 안되는 현상 개선

4. underline 변경은 되돌려지지가 않는 현상 개선

5. undo, redo 실행시 텍스트 객체가 선택되어 있으면 텍스트 객체가 변하면서 UI상태도 변해야 하지만 변하지 않는 버그 수정

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinwoo-kim-nhn Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongsik-yoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinwoo-kim-nhn Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinwoo-kim-nhn Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

jinwoo-kim-nhn Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinwoo-kim-nhn Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinwoo-kim-nhn commented Jan 29, 2020 •

edited

Loading

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading

jinwoo-kim-nhn Feb 4, 2020 •

edited

Loading