Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XML mode for the cell editor #1275

Merged
merged 4 commits into from
Jan 27, 2018
Merged

XML mode for the cell editor #1275

merged 4 commits into from
Jan 27, 2018

Conversation

mgrojo
Copy link
Member

@mgrojo mgrojo commented Dec 23, 2017

This has a lot of changes and new dependencies, so I open a pull request for review and discussion.

The new editor mode shares the same Scintilla widget as the JSON mode.
The JsonTextEdit class has been generalised. Future modes supported by
Scintilla could be added with the current pattern. As a consequence, the
EditMode is not always equal to the current stacked widget.

Some code in EditDialog has been refactored, so it is easier to understand
and modified with so many modes. textNullSet has been replaced by the use
of dataType as Null.

SVG is promoted to a new recognised data type, so it can be edited in the
XML mode.

The XML data is formatted and validated following the pattern established
by the JSON mode.

New modules are needed by the XML mode: the Qt XML module and some new
Scintilla files required by the HTML/XML lexer.

The indent_compact was incorrectly named in Setting::getDefaultValue.

See issue #1253.

The new editor mode shares the same Scintilla widget as the JSON mode.
The JsonTextEdit class has been generalised. Future modes supported by
Scintilla could be added with the current pattern. As a consequence, the
EditMode is not always equal to the current stacked widget.

Some code in EditDialog has been refactored, so it is easier to understand
and modified with so many modes. textNullSet has been replaced by the use
of dataType as Null.

SVG is promoted to a new recognised data type, so it can be edited in the
XML mode.

The XML data is formatted and validated following the pattern established
by the JSON mode.

New modules are needed by the XML mode: the Qt XML module and some new
Scintilla files required by the HTML/XML lexer.

The indent_compact was incorrectly named in Setting::getDefaultValue.

See issue #1253.
@justinclift
Copy link
Member

This sounds pretty awesome @mgrojo. 😄

@MKleusberg
Copy link
Member

Haha, that's a lot of code 😉 I'll try to review it soon-ish but it's going to take some time. I hope that's no problem for you.

@mgrojo
Copy link
Member Author

mgrojo commented Dec 30, 2017

No problem at all. Take the time you deem necessary.

@justinclift justinclift added the enhancement Feature requests. label Jan 21, 2018
Copy link
Member

@MKleusberg MKleusberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you have a look at these comments? I couldn't find any major issues, so they are all minor points. The missing break statements cause double execution of some code and probably don't affect the data, but they are still worth fixing I guess 😄

Are you ok to resolve the merge conflicts too? 😉 I think then we are safe to merge this.

Sorry for taking so long but this huge amount of code was deterring me 😉 Turned out to be mostly Scintilla code but still... I wonder why they need a C++ lexer for the XML lexer...

emit recordTextUpdated(currentIndex, newData.toUtf8(), false);
}
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing break; after this line.


}
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And missing break; after this line.

dataLength = sciEdit->text().length();
break;
}
// ui->labelType->setText(tr("Type of data currently in cell: Text / Numeric"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we delete this line?

Set breaks in nested switch where missing.

Fix editModeChanged since it was overwriting the dataType for JSON and
SVG.

Additionally, removed unnecessary conversion in fromJson.
# Conflicts:
#	src/EditDialog.cpp
#	src/ExtendedScintilla.cpp
@mgrojo mgrojo merged commit c827603 into master Jan 27, 2018
@mgrojo mgrojo deleted the xml_cell_edit branch January 27, 2018 19:09
@justinclift
Copy link
Member

Excellent. Only 2 PR's to go!

I'd better get the SQLite math extension thing figured out soon then. 😄

@mgrojo
Copy link
Member Author

mgrojo commented Jan 29, 2018

@MKleusberg I'm still struggling with git. I had problems merging this and I think I overwrote your change to the qmake compilation, because I don't see it now in this pull request. I'm unable to compile with qmake, so I don't know what I must restore. Sorry. I definitely have to read one or more git tutorials again...

@mgrojo mgrojo mentioned this pull request Jan 29, 2018
@justinclift justinclift restored the xml_cell_edit branch January 29, 2018 20:24
@justinclift
Copy link
Member

@mgrojo I've just restored the deleted branch, so tomorrow (when I'm a bit more awake) I can look into this. If all of the source code bits are still around in places, I should be able to manually reconstruct things into a working order. 😄

And no worries with the git troubles. I friggin hated git when first using it, and it took me ages to grok. But got the hang of it eventually. You'll be fine. 😄

mgrojo added a commit that referenced this pull request Jan 30, 2018
Also, some spaces have been replaced by tabs for uniformity.

See issue #1309
@mgrojo
Copy link
Member Author

mgrojo commented Jan 30, 2018

@justinclift The problem is that overwrote this branch with my local branch. I didn't know why git didn't allow me to push the changes and I made some command for overwriting the remote branch, since I was convinced that the remote branch should be equal to mine. Later I remembered that Martin made some changes to the qmake compilation and then I realized the error 😳 Hopefully that was what I have just committed.

@justinclift justinclift deleted the xml_cell_edit branch January 30, 2018 21:22
@mgrojo mgrojo restored the xml_cell_edit branch October 14, 2023 21:27
@mgrojo mgrojo deleted the xml_cell_edit branch October 14, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants