View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0005917JEDI VCL00 JVCL Componentspublic2012-06-26 11:442015-09-21 17:47
ReporterbheAssigned Toobones 
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Product VersionDaily / GIT 
Target VersionFixed in Version3.49 
Summary0005917: JvGnugettext TranslateStrings Clear has side effects
DescriptionTranslateStrings cares about OwnsObjects but sl.Clear throws all objects away anyway.!

Replacing
          sl.Clear;
          sl.AddStrings(s);
with
          for i := 0 to sl.Count - 1 do
            sl.Strings[i] := s[i];

work well.
This way the OwnsObjects is not required at all.
Additional Information(It looks bit like the return of http://issuetracker.delphi-jedi.org/view.php?id=4273)
TagsNo tags attached.

Activities

bhe

2012-09-26 13:57

reporter   ~0020224

Still not fixed in 3.47! Why?

Thus JvGnugettext is not usable in real world apps:
E.g. not with DevExpress Bars:

TdxBarManagerCategories = class(TStringList)
...
procedure TdxBarManagerCategories.Clear;
begin
  ClearObjects; <- Fatal
  inherited;
end;

Calling Clear for that private class is not provided!

My suggested solution should have less (likely no) side effects.

AHUser

2012-09-26 18:52

developer   ~0020225

> Still not fixed in 3.47! Why?

Because nobody had a look at this. There are not many left who work on the JCL and JVCL code and those who are, don't spend their whole spare time on it.



> My suggested solution should have less (likely no) side effects.

Your changes have the effect that you can end up with a messed up translation for a sorted string list (assigning a translated string to Strings[] changes the order of the strings in the list).

I have now added code to set all sl.Objects[] to nil before calling sl.Clear. That should fix the bug and not introduce the "sorted string list" issue.

The svn revision is 13449. I leave that report open so you can add comment it (in case that fix doesn't help you)

bhe

2012-09-26 23:41

reporter   ~0020228

Why not temporary setting Sorted to False?
Or special dealing for sorted and unsorted lists?

"Niling" the Objects might work (likely it will in the sample case).
But you never know what happens in an derived class
if PutObject() is overridden.

Btw. auto translation of a sorted string list in a persistent component
likely will not work in practical cases.
Maybe because of this i had no issues with my solution.

AHUser

2012-09-27 09:49

developer   ~0020229

The older Mantis issue 4273 explicitly tests for the ClassType. Maybe we should restore that code, that got lost when the JvGnuGetText was synchronized with the dxgettext (http://dxgettext.po.dk/) code. The older code also just ignored the Sorted property for "unknown" ClassTypes.

      if (sl.ClassType = TStrings) or (sl.ClassType = TStringList) or
         (sl.ClassName = 'TMemoStrings') or (sl.ClassName = 'TComboBoxStrings') or
         (sl.ClassName = 'TListBoxStrings') then

bhe

2012-09-27 12:49

reporter   ~0020235

That could be a way to go - at least i had no issue with that implementation.

Just ignoring the Sorted prop can lead to an EStringListError(SSortedListError).
Perhaps a dedicated assertion for this case could be a good indication for those who using GetText in such a questionable way.

bhe

2012-10-26 01:37

reporter   ~0020270

Their is a small "typo" in:

          if (sl.ClassType <> TStringList) and (sl is TStringList) then
            for I := 0 to sl.Count do
              sl.Objects[I] := nil;
Has to be:
          if (sl.ClassType <> TStringList) and (sl is TStringList) then
            for I := 0 to sl.Count - 1 do
              sl.Objects[I] := nil;

Something happens to You!? ;-)

Besides that, the initial scenario work with that solution!
So far so good.

Nevertheless i prefer my simple assign approach for unsorted list:

          for i := 0 to sl.Count - 1 do
            sl[i] := s[i];

Thus avoids calls to Clear and AddStrings - who knows how they are overwritten. Should be slightly faster too.

obones

2013-01-15 15:01

administrator   ~0020336

Could you look at the publicly available gnugettext to see how they do it? I'm quite sure there were fixes in it that we did not incorporate back

bhe

2013-01-16 14:01

reporter   ~0020401

You mean http://dxgettext.svn.sourceforge.net/viewvc/dxgettext/trunk/dxgettext?

They just care about OwnsObjects but not about Sorted.
(Which was never an issue for me - see statement from 2012-09-26 23:41)

But they use Clear + AddStrings which fails!

My productive code look like that {Comments removed):

procedure TGnuGettextInstance.TranslateStrings(sl: TStrings;const TextDomain:DomainString);
...
        {$ifdef DELPHI2009OROLDER}
        if Assigned(slAsTStringList) then begin
          originalOwnsObjects := slAsTStringList.OwnsObjects;
          slAsTStringList.OwnsObjects := False;
        end;
        {$endif}
        try
          if(sl is TStringList)and TStringList(sl).Sorted then
          begin
            if (sl.ClassType <> TStringList) and (sl is TStringList) then
              for I := 0 to sl.Count - 1 do
                sl.Objects[I] := nil;
            sl.Clear;
            sl.AddStrings(s);
          end else
            for i := 0 to sl.Count - 1 do
              sl[i] := s[i];
        finally
          {$ifdef DELPHI2009OROLDER}
          if Assigned(slAsTStringList) then
            slAsTStringList.OwnsObjects := originalOwnsObjects;
          {$endif}
        end;

(Used in several successful applications. One of them with 1.3 Mio LoC
got more than 1 Mio downloads and is running in 13 languages.)

obones

2013-12-16 17:03

administrator   ~0020867

I'm confused.
You say that Clear + AddStrings fails when Sorted is True, and yet, your "production" code calls Clear + AddStrings when the list is sorted.
Which one is the correct one?

bhe

2013-12-17 00:38

reporter   ~0020871

Your are right - but AFTER setting all objects to Nil.
Here a commented version:

if(sl is TStringList)and TStringList(sl).Sorted then
begin
  // Classes inheriting from TStringList might free objects in overriden Clear()
  // Thus we remove all object before calling Clear()
  if (sl.ClassType <> TStringList) and (sl is TStringList) then
    for I := 0 to sl.Count - 1 do
      sl.Objects[I] := nil;
  sl.Clear;
  sl.AddStrings(s); // This restores Objects too!
end else
  for i := 0 to sl.Count - 1 do
    sl[i] := s[i];

obones

2013-12-18 15:28

administrator   ~0020880

Ok, fair enough, but when the string list is not sorted, this means we use the "sl[i] := s[i]" construct, and thus we loose the objects that AddStrings would have restored.

bhe

2013-12-18 17:29

reporter   ~0020889

Why? TStringList.Put() does not effect the objects.

Btw. the OwnsObjects handling is IMHO only required in the Sorted case.
OwnsObjects (in TStringList) only matters for Clear, Delete and Destroy.

obones

2013-12-24 16:41

administrator   ~0020891

Yes, you are right.
This is now in GIT.

Issue History

Date Modified Username Field Change
2012-06-26 11:44 bhe New Issue
2012-09-26 13:57 bhe Note Added: 0020224
2012-09-26 18:52 AHUser Note Added: 0020225
2012-09-26 18:52 AHUser Status new => feedback
2012-09-26 23:41 bhe Note Added: 0020228
2012-09-27 09:49 AHUser Note Added: 0020229
2012-09-27 12:49 bhe Note Added: 0020235
2012-10-26 01:37 bhe Note Added: 0020270
2013-01-15 15:01 obones Note Added: 0020336
2013-01-16 14:01 bhe Note Added: 0020401
2013-12-13 11:40 obones Status feedback => acknowledged
2013-12-16 17:03 obones Note Added: 0020867
2013-12-16 17:03 obones Status acknowledged => feedback
2013-12-17 00:38 bhe Note Added: 0020871
2013-12-18 15:28 obones Note Added: 0020880
2013-12-18 17:29 bhe Note Added: 0020889
2013-12-24 16:41 obones Note Added: 0020891
2013-12-24 16:41 obones Status feedback => resolved
2013-12-24 16:41 obones Fixed in Version => Daily / GIT
2013-12-24 16:41 obones Resolution open => fixed
2013-12-24 16:41 obones Assigned To => obones
2015-09-21 17:47 obones Fixed in Version Daily / GIT => 3.49