TDE
/
gtk-qt-engine
6
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

Labels Milestones

KCM: Make sure Unicode is used for editing bashrc (fixes issue #6) #7

Merged
blu.256 merged 1 commits from fix/issue-6 into master 3 years ago
Conversation 4 Commits 1 Files Changed 1
blu.256 commented 3 years ago
Collaborator

This is a proposed fix for issue 6.

This resolves issue #6.

This is a proposed fix for issue 6. This resolves issue #6.
blu.256 added 1 commit 3 years ago
51384f5743
KCM: Make sure Unicode is used for editing bashrc 
This resolves issue #6.

Signed-off-by: Mavridis Philippe <mavridisf@gmail.com>
blu.256 requested review from SlavekB 3 years ago
blu.256 added this to the R14.0.12 release milestone 3 years ago
SlavekB requested changes 3 years ago
SlavekB left a comment
Owner

Please do the test using locales encoding.
See comment.

Please do the test using locales encoding. See comment.
kcm_gtk/kcmgtk.cpp Outdated
@ -511,2 +511,3 @@
file.open(IO_ReadOnly);
TQByteArray fileData = file.readAll();
TQTextStream fileDataStream = TQTextStream(&file);
fileDataStream.setEncoding(TQTextStream::UnicodeUTF8);
SlavekB commented 3 years ago
Owner

The use of stream encoding is a good choice, but I believe that the locale encoding should be used: TQTextStream::Locale. It is likely that the UTF8 encoding will be currently used for most languages, but there may be exceptions.

The use of stream encoding is a good choice, but I believe that the _locale_ encoding should be used: `TQTextStream::Locale`. It is likely that the UTF8 encoding will be currently used for most languages, but there may be exceptions.
blu.256 commented 3 years ago
Poster
Collaborator

I based this off some code I found in BasKet, so maybe this should be fixed in other places as well.
Good point.

I based this off some code I found in BasKet, so maybe this should be fixed in other places as well. Good point.
SlavekB marked this conversation as resolved
blu.256 force-pushed fix/issue-6 from 51384f5743 to a7d117f83a 3 years ago
blu.256 commented 3 years ago
Poster
Collaborator

After testing TQTextStream::Locale works as expected.

After testing `TQTextStream::Locale` works as expected.
blu.256 requested review from SlavekB 3 years ago
SlavekB reviewed 3 years ago
kcm_gtk/kcmgtk.cpp
@ -518,3 +519,3 @@
file.open(IO_WriteOnly);
stream.setDevice(TQT_TQIODEVICE(&file));
SlavekB commented 3 years ago
Owner

One more idea: Encoding should also be set when writing data back to file?

One more idea: Encoding should also be set when writing data back to file?
blu.256 commented 3 years ago
Poster
Collaborator

I don't know; right now it works well. Maybe encoding should only be set when reading from stream?

According to the TQt docs,

By default, output of Unicode text (i.e. TQString) is done using the local 8-bit encoding.

so if I understood that well, it sets the appropriate encoding automatically for the output.

I don't know; right now it works well. Maybe encoding should only be set when reading from stream? According to the TQt docs, > By default, output of Unicode text (i.e. TQString) is done using the local 8-bit encoding. so if I understood that well, it sets the appropriate encoding automatically for the output.
SlavekB commented 3 years ago
Owner

Ok, thank you, in this case it should probably be fine.

Ok, thank you, in this case it should probably be fine.
SlavekB marked this conversation as resolved
SlavekB approved these changes 3 years ago
SlavekB left a comment
Owner

It looks fine.
Thank you for fast fix!

It looks fine. Thank you for fast fix!
blu.256 commented 3 years ago
Poster
Collaborator

Merging and backporting.
No squash needed.

Merging and backporting. No squash needed.
blu.256 merged commit a7d117f83a into master 3 years ago
blu.256 deleted branch fix/issue-6 3 years ago
SlavekB commented 3 years ago
Owner

Problem – I didn't make building before, but now, after merge, and FTBFS occurs:

../kcm_gtk/kcmgtk.cpp: In member function 'virtual void KcmGtk::save()':
../kcm_gtk/kcmgtk.cpp:512:51: error: 'TQTextStream::TQTextStream(const TQTextStream&)' is priva
te within this context
  512 |   TQTextStream fileDataStream = TQTextStream(&file);
      |                                                   ^
In file included from /usr/include/tqt3/ntqtl.h:46,
                 from /usr/include/tqt3/ntqvaluelist.h:45,
                 from /usr/include/tqt3/ntqmap.h:49,
                 from /usr/include/tqt3/ntqmime.h:46,
                 from /usr/include/tqt3/ntqevent.h:48,
                 from /usr/include/tqt3/ntqobject.h:48,
                 from /usr/include/tqt3/ntqlayout.h:45,
                 from /usr/include/tqt/tqlayout.h:32,
                 from ../kcm_gtk/kcmgtk.cpp:21:
/usr/include/tqt3/ntqtextstream.h:197:5: note: declared private here
  197 |     TQTextStream( const TQTextStream & );
      |     ^~~~~~~~~~~~
Problem – I didn't make building before, but now, after merge, and FTBFS occurs: ``` ../kcm_gtk/kcmgtk.cpp: In member function 'virtual void KcmGtk::save()': ../kcm_gtk/kcmgtk.cpp:512:51: error: 'TQTextStream::TQTextStream(const TQTextStream&)' is priva te within this context 512 | TQTextStream fileDataStream = TQTextStream(&file); | ^ In file included from /usr/include/tqt3/ntqtl.h:46, from /usr/include/tqt3/ntqvaluelist.h:45, from /usr/include/tqt3/ntqmap.h:49, from /usr/include/tqt3/ntqmime.h:46, from /usr/include/tqt3/ntqevent.h:48, from /usr/include/tqt3/ntqobject.h:48, from /usr/include/tqt3/ntqlayout.h:45, from /usr/include/tqt/tqlayout.h:32, from ../kcm_gtk/kcmgtk.cpp:21: /usr/include/tqt3/ntqtextstream.h:197:5: note: declared private here 197 | TQTextStream( const TQTextStream & ); | ^~~~~~~~~~~~ ```
SlavekB reviewed 3 years ago
kcm_gtk/kcmgtk.cpp
@ -510,3 +510,3 @@
{
file.open(IO_ReadOnly);
TQByteArray fileData = file.readAll();
TQTextStream fileDataStream = TQTextStream(&file);
SlavekB commented 3 years ago
Owner

Now I found that there is already TQTextStream stream(&file); for file defined above. So you can use the previous stream variable. Instead of a new fileDataStream.
I'll do a test if it also solves the FTBFS, which I watch now.

Now I found that there is already `TQTextStream stream(&file);` for `file` defined above. So you can use the previous `stream` variable. Instead of a new `fileDataStream`. I'll do a test if it also solves the FTBFS, which I watch now.
blu.256 commented 3 years ago
Poster
Collaborator

I just checked with the following changes:

diff --git a/kcm_gtk/kcmgtk.cpp b/kcm_gtk/kcmgtk.cpp
index bd3ab79..9822b30 100644
--- a/kcm_gtk/kcmgtk.cpp
+++ b/kcm_gtk/kcmgtk.cpp
@@ -509,9 +509,9 @@ void KcmGtk::save()
        if (file.exists())
        {
                file.open(IO_ReadOnly);
-               TQTextStream fileDataStream = TQTextStream(&file);
-               fileDataStream.setEncoding(TQTextStream::Locale);
-               TQString fileDataString = fileDataStream.read();
+               stream.setDevice(TQT_TQIODEVICE(&file));
+               stream.setEncoding(TQTextStream::Locale);
+               TQString fileDataString = stream.read();
                file.close();

                TQString rcLine = "export GTK2_RC_FILES=$HOME/.gtkrc-2.0";

Builds and works as expected.

I just checked with the following changes: ``` diff --git a/kcm_gtk/kcmgtk.cpp b/kcm_gtk/kcmgtk.cpp index bd3ab79..9822b30 100644 --- a/kcm_gtk/kcmgtk.cpp +++ b/kcm_gtk/kcmgtk.cpp @@ -509,9 +509,9 @@ void KcmGtk::save() if (file.exists()) { file.open(IO_ReadOnly); - TQTextStream fileDataStream = TQTextStream(&file); - fileDataStream.setEncoding(TQTextStream::Locale); - TQString fileDataString = fileDataStream.read(); + stream.setDevice(TQT_TQIODEVICE(&file)); + stream.setEncoding(TQTextStream::Locale); + TQString fileDataString = stream.read(); file.close(); TQString rcLine = "export GTK2_RC_FILES=$HOME/.gtkrc-2.0"; ``` Builds and works as expected.
SlavekB commented 3 years ago
Owner

For me build was successful only on Debian 12 (Bookworm), Ubuntu 21.10 (Impish) and Ubuntu 22.04 (Jammy). With an update in #8 build is successfull also on Debian 9 (Stretch).

For me build was successful only on Debian 12 (Bookworm), Ubuntu 21.10 (Impish) and Ubuntu 22.04 (Jammy). With an update in #8 build is successfull also on Debian 9 (Stretch).
blu.256 commented 3 years ago
Poster
Collaborator

Strange, it builds fine on Slackware.

Strange, it builds fine on Slackware.

Reviewers

SlavekB approved these changes 3 years ago
The pull request has been merged as a7d117f83a.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
SlavekB
Labels
Apply labels
Clear labels   
GE/need-info

General - need additional info from contributor   
PR/keep-branch

Pull request - do not delete branch after merging   
PR/not-ok

Pull request - need fixing   
PR/rfc

Pull request - request for comments   
PR/update-trans

Pull request - update to translation files needed   
PR/wip

Pull request - work in progress   
RS/R14.0.x

Related to R14.0.x series   
RS/R14.1.x

Related to R14.1.x series   
SL/critical

Severity level - critical   
SL/major

Severity level - major   
SL/minor

Severity level - minor   
SL/normal

Severity level - normal   
SL/regression

Severity level - regression from previous version   
SL/trivial

Severity level - trivial   
SL/wishlist

Severity level - wishlist request   
ST/duplicate

Status - duplicate of another issue   
ST/invalid

Status - invalid report   
ST/notourproblem

Status - not our problem   
ST/rejected

Status - rejected   
ST/wontfix

Status - won't fix   
ST/worksforme

Status - works for me, unable to reproduce
No Label
GE/need-info
PR/keep-branch
PR/not-ok
PR/rfc
PR/update-trans
PR/wip
RS/R14.0.x
RS/R14.1.x
SL/critical
SL/major
SL/minor
SL/normal
SL/regression
SL/trivial
SL/wishlist
ST/duplicate
ST/invalid
ST/notourproblem
ST/rejected
ST/wontfix
ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.0.12 release
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: TDE/gtk-qt-engine#7
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'fix/issue-6'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes