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

Don't trim items when exploding to string list for export to RTF #927

Merged
merged 1 commit into from Mar 5, 2020

Conversation

@igitur
Copy link
Contributor

igitur commented Mar 4, 2020

Currently, when copying a SQL script from the SynEdit control, the RTF part loses indentation, e.g. if I have this script:

CREATE TABLE /*!32312 IF NOT EXISTS*/ tableB (
  id INT,
  name VARCHAR(30) DEFAULT "standard"
)

and I copy it to the clipboard , then the RTF part is

{\rtf1\ansi\ansicpg1252\uc1\deff0\deftab720{\fonttbl{\f0\fmodern Courier New;}}
{\colortbl\red0\green0\blue255;\red0\green0\blue0;\red128\green128\blue128;\red128\green128\blue0;\red128\green0\blue0;\red128\green0\blue128;}
{\info{\comment Generated by the SynEdit RTF exporter}
{\title Untitled}}
\deflang1033\pard\plain\f0\fs20 \cf0\b CREATE\b0\cf1  \cf0\b TABLE\b0\cf1  \cf2\i /*!32312 IF NOT EXISTS*/\i0\cf1  \cf3 tableB\cf1  \cf0 (
\par \cf3 id\cf1  \cf4\b INT\b0\cf0 ,
\par \cf3 name\cf1  \cf4\b VARCHAR\b0\cf0 (\cf5 30\cf0 )\cf1  \cf0\b DEFAULT\b0\cf1  \cf3 "standard"
\par \cf0 )
\par }

which renders to:
image

This PR adds an optional parameter to the Explode method to optionally not trim the items before being added to the string list. When copying, this optional parameter is set to false. All other uses of the method is unaffected.

After the fix, RTF rendered is:
image

@ansgarbecker

This comment has been minimized.

Copy link
Collaborator

ansgarbecker commented Mar 4, 2020

Thanks for finding that. Reading the changes, I think I should remove that trim() without introducing an optional parameter. This trimming is just wrong, from the beginning.

@igitur

This comment has been minimized.

Copy link
Contributor Author

igitur commented Mar 4, 2020

I also considered that, but Explode is used so much that I found it too risky for me to remove trim(). I don't know the codebase that well enough.

@ansgarbecker

This comment has been minimized.

Copy link
Collaborator

ansgarbecker commented Mar 4, 2020

I am sure I did not really think about the consequences of that trim(), back in 2006, before I even used a versioning system. This is how explode() looked like in my backuped code from April 2006:

// explode a string by separator into a TStringList
function explode(separator, a: String) :TStringList;
var
  i : Integer;
  item : String;
begin
  result := TStringList.Create();

  i := pos(separator, a);
  while i > 0 do begin
    item := copy(a, 0, i-1);
    item := trim(item);
    result.Add(item);
    a := copy(a, i+length(separator), length(a));
    i := pos(separator, a);
  end;
  if a <> '' then
    result.Add(trim(a));
end;
@igitur igitur force-pushed the igitur:retain-indentation-in-rtf branch from c984a69 to dee3473 Mar 4, 2020
@igitur igitur force-pushed the igitur:retain-indentation-in-rtf branch from dee3473 to 26e8f67 Mar 4, 2020
@igitur

This comment has been minimized.

Copy link
Contributor Author

igitur commented Mar 4, 2020

Ok, I updated the PR to just remove the trim().

@ansgarbecker ansgarbecker merged commit 64c405f into HeidiSQL:master Mar 5, 2020
@igitur

This comment has been minimized.

Copy link
Contributor Author

igitur commented Mar 5, 2020

By the way, Rebase leads to cleaner history than a Merge, especially for a small PR like this.

@igitur igitur deleted the igitur:retain-indentation-in-rtf branch Mar 5, 2020
@ansgarbecker

This comment has been minimized.

Copy link
Collaborator

ansgarbecker commented Mar 5, 2020

Yes I know. I thought I clicked "Rebase and merge"..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.