patchGNU Octave - Patches: patch #7857, Replace dynamic casts of GUI/IDE...

 
 

patch #7857: Replace dynamic casts of GUI/IDE editor with more slots/signals configuration

Submitter:  None
Submitted:  Wed 03 Oct 2012 10:42:40 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  ttl Originator Email:  -email is unavailable-
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 25 Dec 2012 09:25:13 PM UTC, comment #28: 

Thanks a lot, Dan. Pushed with changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/424edeca3c66

Torsten Lilge <ttl>
Group Member
Sun 23 Dec 2012 08:51:30 PM UTC, comment #27: 

I'm fine with making the external file change a separate patch.  In fact, it is probably best to have a good working version the first way, then a small change for the second way.  I could make the change, but unless I can verify proper behavior either by some test on my system or by Qt documentation it probably makes no sense to change it.

I've created a new patch with what I think are the things I touched in the commit message.  (Sure was a mess...must have reverted some changes you made by forcing a forward commit.)  This seems to restore desired behavior.  Let me know if it doesn't work correctly.

On Windows systems case is ignored?  I sort of remember that for DOS, but I'd have thought that was changed given that spaces were allowed in file names.  Let's make that a separate commit.  I don't see that I altered anything with "WINDOWS", etc.  Mainly this patch is to get things more in line with signals/slots and fix the few obvious problems with saving and opening files in the editor.

Sorry this took a while.  Octave has been somewhat unstable for a month or so and I thought I'd wait for things to even out a bit.

(file #27147)

Dan Sebald <sebald>
Sat 22 Dec 2012 06:57:23 AM UTC, comment #26: 

Dan, maybe the file buffering problem should be handled separately from this patch?

Torsten Lilge <ttl>
Group Member
Fri 23 Nov 2012 07:31:46 PM UTC, comment #25: 

Yes, I think you are correct.  These hunks are inadvertently in the wrong place somehow.  I don't recall ever editing anything near floating code, and that I'd delete something in relation to float is doubtful.

I guess I didn't understand the mercurial import feature well enough.  I should have just gone back to the tried and true manually replacing old hunks.  I wish the "diff" algorithm were a little more selective and not create such big hunks that can easily conflict with other changes.

I will look at this this weekend.  I want to review the Qt file buffering to see if there is any risk of missing an exterior file change.

Dan Sebald <sebald>
Fri 23 Nov 2012 01:18:51 PM UTC, comment #24: 

Daniel, it turned out that the patch disables the feature that the editor becomes a real window when floating. The related lines of the newest patch file are:
2185-2192, 2266-2271, 2447-2448, 2490-2494, 2514-2520
I guess this is not intended but happened in some way by creating the new version of the patch?

Is it possible to split up the patch into several smaller ones? Maybe it would be easier then to ensure that there are no other accidental changes.

Torsten Lilge <ttl>
Group Member
Sun 11 Nov 2012 03:55:38 PM UTC, comment #23: 

I would like to push this patch. Are there any other comments?

Two more comments from my side:

In check_conflict_save

  • the test is case sensitive; this could raise problems on windows where file names are not case sensitive
  • I vote for not allowing writing in case of conflict, just a message box


Torsten Lilge <ttl>
Group Member
Tue 06 Nov 2012 05:57:02 PM UTC, comment #22: 

Great!  It wasn't anything tricky then.  I'm guessing your Qt version is slightly more recent than mine and developers made the file watcher slightly more sensitive--probably open for writing is a "change-on-disk" but not open for reading.

Note that each file_editor_tab has its own file watcher member.  One file watcher might be enough, but that change is low priority and should be done as a single change.

Dan Sebald <sebald>
Tue 06 Nov 2012 05:41:36 PM UTC, comment #21: 

Yes, no more messages when saving a file. Thanks!



Torsten Lilge <ttl>
Group Member
Tue 06 Nov 2012 10:03:51 AM UTC, comment #20: 

Give the attached patch a try.  It used to be that the to-be-saved file was removed from watch after open but before save.  I moved the watch file removal to before file.open ().  My thinking is that opening a file could be, by some definitions, a change of file on disk.  Perhaps Qt developers made a subtle change regarding that detail between versions.

[now appears before file.open]
  // stop watching file
  QStringList trackedFiles = _file_system_watcher.files ();
  if (!trackedFiles.isEmpty ())
    _file_system_watcher.removePath (saveFileName);

  // open the file for writing
  QFile file (saveFileName);
  if (!file.open (QIODevice::WriteOnly))
    {
[added this and confirmed it works be opening a read-only file and attempting a save]
      // Unsuccessful, begin watching file again if it was being
      // watched previously.
      if (trackedFiles.contains (saveFileName))
        _file_system_watcher.addPath (saveFileName);



(file #26880)

Dan Sebald <sebald>
Tue 06 Nov 2012 01:50:22 AM UTC, comment #19: 

If this was working for you before, I doubt it would be a buffering then vs. now issue.  Does your .config/octave/qt-settings file have a reasonable sessions description?  E.g.,

savedSessionTabs=/home/sebald/octave/junk.m, /home/sebald/octave/junkjunk.m

I'm wondering if perhaps there is a duplicate copy in your list somehow from running a previous version that allowed multiple copies to be opened.  If so, maybe there is a bug at startup not accounted for.

I'll keep thinking of other alternatives.

The "save_file" routine isn't the cleanest approach, but I didn't want to shuffle that around too much.  Could possibly come up with a slightly different approach.

Dan Sebald <sebald>
Mon 05 Nov 2012 09:13:33 PM UTC, comment #18: 

I am running Linux, too (ubuntu 12.04 32 bit). I tested the mode options you mentioned, without success. This is really odd, especially because it worked before and I can't find a relevant difference in how the watcher is used.

Torsten Lilge <ttl>
Group Member
Mon 05 Nov 2012 10:26:35 AM UTC, comment #17: 

I'm not seeing this.  Strange.  I'm running under Linux.  Is that what you are running under?  (Does your editor have the same file open twice perhaps by having the same name in the settings file as a result of debugging?)

This command should flush and close the file:

  file.close();

before making the name change and putting the file name into the watcher.  But that doesn't appear to be happening for your compilation.  There are some circumstances where a close isn't done, just a flush, but that is for things like stderr, stdout.  It is as though the flush/close is being delayed in your case.

I do notice what may be a problem.  When the file is opened, the enumeration parameter doesn't seem to be anything valid:

  // open the file for writing
  QFile file (saveFileName);
  if (!file.open (QFile::WriteOnly))
    {

There is no such QFile::WriteOnly.  There is a QIODevice::WriteOnly.  Perhaps the C++ compiler thinks that QFile::WriteOnly is the definition of a QFile object called "WriteOnly" and is passing in some strange value as an enumeration.  I've attached a new patch that makes this one change in addition to the previous:

  // open the file for writing
  QFile file (saveFileName);
  if (!file.open (QIODevice::WriteOnly))
    {

Please give that a try.

If that doesn't help, there may be one other thing to try here.  I see in the list of flags QIODevice::Unbuffered.  Perhaps this scenario needs the I/O unbuffered so that when file.close is done there is no delay (and on my system these files are inherently non-buffered already, maybe your system different), just hypothesising.  So, if the patch doesn't work, try changing the conditional line to

  if (!file.open (QIODevice::WriteOnly | QIODevice::Unbuffered))



(file #26876)

Dan Sebald <sebald>
Sun 04 Nov 2012 08:05:05 PM UTC, comment #16: 

Thanks for the new patch.
It fixes the issues mentioned in my earlier post except for the warning that another application has changed the file when saving. This does not depend on the way the file was opened or created.

I did not find an obvious reason for this effect since the handling of the file watcher has not changed but there is an interesting effect: If you put
_file_system_watcher.addPath (saveFileName);
in save_file() (not in set_file_name()) directly after
set_file_name(savedFileName);
you can save the file once without warning but following save actions produce the warning again.

Torsten Lilge <ttl>
Group Member
Wed 31 Oct 2012 08:37:23 PM UTC, comment #15: 

I missed line breaks in some lines.  New patch.

(file #26863)

Dan Sebald <sebald>
Wed 31 Oct 2012 04:16:57 PM UTC, comment #14: 

Hold on.  I found the problem with checksum.

> Importing seems to work here:
> Mercurial Distributed SCM (version 1.8.4)


I forgot that on my system I had commented out the stderr redirection in octave/libgui/libqterminal/unix

QUnixTerminalImpl.cpp://        qDebug("Error: stderr is not a tty.");

That portion of the repository is under git.  So in the changeset, it is only the checksum file that is processed by the "hg diff", i.e., commit.  So hidden within that recent patch set is this:

diff -r df781031095a -r 76e3918e5414 .hgsubstate
--- a/.hgsubstate Thu Oct 04 00:52:50 2012 -0500
+++ b/.hgsubstate Mon Oct 29 17:31:36 2012 -0500
@@ -1,2 +1,2 @@
 1af55d85d9762a679b4302d5995f05ccd883e956 gnulib
-0820083f7fe95d0088971b1233540c7827218e7c libgui/qterminal
+c3cc74a93534854f468ceb81177b5c2b7a35c73b libgui/qterminal

That the correct checksum for me, but of course not for you ttl.

Glad that is cleared up.  I was worried I wasn't doing multiple changesets properly.

I've attached a patch that lumps everything together and adds line breaks to the commit comments.  No checksum hunk.

(file #26862)

Dan Sebald <sebald>
Wed 31 Oct 2012 06:50:09 AM UTC, comment #13: 

Importing seems to work here:

Mercurial Distributed SCM (version 1.8.4)

Dan Sebald <sebald>
Wed 31 Oct 2012 06:47:45 AM UTC, comment #12: 

That's not good.

Something isn't right.  I might be doing something very wrong with the way I imported an old changeset, made more changes, then exported several.  However, that sure would be the way to go so that multiple programmers can just add to a changeset.

I think for the time being I will just lump all changes onto a fresh clone.  But that checksum error you are getting shouldn't happen; I can understand other sorts of quirks but a checksum usually means only one thing: the file is corrupted, e.g., not fully downloaded.

JGH, as regards to the word wrap, I'm using gvim, vim or vi.  I'll either edit in gvim and then copy into the editor that pops up during "commit" (which I think is vim) or I'll type a short message in the editor from "commit".  gvim/vim has a word wrap setting (no line breaks) and line wrap (line breaks).  I prefer no line breaks.  But I can add line breaks right before commit.

I was worried that the

hg log -v --style changelog

wouldn't look so good with line breaks.  However, I just did a test run with a message that has several line breaks and short lines.  Mercurial must be set up to remove all those when it constructs it's commit message.

It would be nice if there were away to apply the "hg log" command to a changeset file, but I don't see any.

Dan Sebald <sebald>
Wed 31 Oct 2012 05:53:28 AM UTC, comment #11: 

Then there might be a hg bug, particularly as it turned out that my repo was unusable afterwards (hg log leads to 'unknown error' and a bunch of following messages). This even happens with a fresh repo after trying to import the patch.

Torsten Lilge <ttl>
Group Member
Tue 30 Oct 2012 09:38:34 PM UTC, comment #10: 


> hg export 15565 15566 15567 15568


If that's what you did, then I suppose it was intentional to have several commits in the same. I can't understand what the problem was. Perhaps a separate hg bug? I don't know.

A request, can you please word-wrap your commit messages? There are no newlines in them and they look like very long lines. What editor are you using to write them?

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Tue 30 Oct 2012 08:27:10 PM UTC, comment #9: 

The bug report makes it sound like there shouldn't be multiple "# HG changeset patch" in a file, but my thinking was that is desired, analogous to "push" or "pull" in which multiple changesets transfer.  I can't recall the exact command, but I used something like:

hg export 15565 15566 15567 15568

after having created several small changes with commit notes for each rather than one big commit.

I might not completely understand the nomenclature, so things aren't as clear as they could be for me on this.  "changeset" I would take to mean, multiple patches, and patch being multiple hunks.

Basically, the changes you made make it look like just one changeset, which is fine.  However, the import modifies 94 files

[dan octave]# hg import dan.patch --exact
applying dan.patch
94 files updated, 0 files merged, 3 files removed, 0 files unresolved

which I don't understand.

I will look at this later in the day and create a single changeset for the whole thing from a fresh clone.

Dan Sebald <sebald>
Tue 30 Oct 2012 07:40:49 PM UTC, comment #8: 

Daniel, there's a bug currently in Mercurial where "hg export -o" appends instead of overwrite:

http://bz.selenic.com/show_bug.cgi?id=3652

For this reason, your patch does not apply since the file contains 4 different commmits. I removed all commits except the first one and attach them here.

It is possible to put more than one commit in the same patch, but I don't think this was your intention. The other commits seem unrelated.

> is there a way to configure another language for hg?


Torsten, change your LANG environment variable. It's apparently set to de_DE.UTF-8 or something like it. Change it to the name of an English locale or to C for no locale.


(file #26858)

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Tue 30 Oct 2012 06:42:30 PM UTC, comment #7: 

Sorry, I can't import the patch. hg says:

Abort: Checksum does not match: Patch is corrupt

(translated from German,
is there a way to configure another language for hg?).

Torsten Lilge <ttl>
Group Member
Mon 29 Oct 2012 11:37:21 PM UTC, comment #6: 

This "hg import --exact" is pretty nice.  It appears that the file patches cleanly and is automatically rebased.  I suppose why this works is that the diff files can be pretty illogical sometimes, with big hunks of code removed then added somewhere else.  I'm going to guess that Mercurial pushes the imported changeset as far forward as possible retaining no loss of hunks, not certain.

Anyway, thanks for the bug reports.  What I've done is added changesets to the original patch.  So the attached patch has four changesets within.  Let me know how well Mercurial applies this changeset via "import".  There are so many alternatives for Mercurial changesets, let's see how this works.  If this turns out to be dodgy, I can create a single up-to-date changeset in the historical way.


> - The setting "Show complete path in window title" is ignored (there are both

at the same time, short and long titles)

This is a two line fix.  The file_editor was initially supplying the name to the command which is always full path.  One solution would be to remove the path inside file_editor, but I prefer supplying an empty file name then have file_editor_tab update the name, i.e., code re-use instead of duplication.

On this matter, what I'd kind of like is a short file name in the tab entry, and then when positioning mouse cursor over tab a dialog pops up that contains the full path name; tough to do though because tab object is part of the Qt library.  Would have to create a new object derived from the tab container with an override of the display function.


> - It is not possible to run a script from the editor (Ctrl-R)


Oversight.  In the original patch I removed the _editor indirection of file_editor_tab but forgot to implement what used to be _editor->terminal()->sendText().  I simply connected a new signal for file_editor_tab to the existing handle_command_double_clicked (misnomer, as "double click" hasn't much to do with the function) of the main_window:

  connect (f, SIGNAL (process_octave_code (const QString&)),
           parent (), SLOT (handle_command_double_clicked (const QString&)));

which is a pretty simple and elegant solution--code reuse and signal/slot paradigm.  (The double-click in the history has problems, BTW...but that doesn't have much to do with this changeset.)


> - Directly after octave came up a file can be opened several times in the

editor wihtout error message; only after a "Save as" the check for multiple
open files seems to work

Good catch.  The fetFileNames list is not updated at initialization.  Rather than do that at startup, I moved a list clear/generate inside request_open_file:

      // Have all file editor tabs signal what their file names are.
      fetFileNames.clear ();
      emit fetab_file_name_query (0);


> - After having closed all open files in the editor I get the message that the

file is already open while trying to reopen one of the files previously
closed

I believe this is the same problem as previous, only manifested slightly different.  Whereas in the previous case the file list was empty so kept allowing the same file to be opened, after the close the file list was bogus and wouldn't allow closed files to be re-opened.


> - Saving a file ("Save", not "Save as") leads to the message that another

application has changed the file

I don't see this problem.  Perhaps you can be more descriptive of exactly how you opened/created the file tab before attempting a "Save" (e.g., created new, opened existing file).


(file #26854)

Dan Sebald <sebald>
Fri 26 Oct 2012 07:12:35 PM UTC, comment #5: 


> Since there were some new revisions in the meantime the import of
> the patch aborted and I had to resolve some conflicts by hand.


Do "hg import --exact" to apply the patch at the location where it was
created. If the patch isn't corrupt, it will apply cleanly there. Then
you can do "hg rebase" to move it to the new location and use your
usual merge tools to fix whatever conflicts there may have been.


Jordi Gutiérrez Hermoso <jordigh>
Group Member
Fri 26 Oct 2012 07:08:14 PM UTC, comment #4: 

I imported the patch and played around a little bit. The new structure looks promising for implementing new features or fixing bugs.

Here are some issues I encountered:

- The setting "Show complete path in window title" is ignored (there are both at the same time, short and long titles)

- It is not possible to run a script from the editor (Ctrl-R)

- Directly after octave came up a file can be opened several times in the editor wihtout error message; only after a "Save as" the check for multiple open files seems to work

- Saving a file ("Save", not "Save as") leads to the message that another application has changed the file

- After having closed all open files in the editor I get the message that the file is already open while trying to reopen one of the files previously closed

Since there were some new revisions in the meantime the import of the patch aborted and I had to resolve some conflicts by hand. Is there a need for a diff-file related to the actual revision?


Torsten Lilge <ttl>
Group Member
Thu 04 Oct 2012 05:57:03 AM UTC, comment #3: 

This updated patch adds an else condition in the file_editor_tab::closeEvent() function.

(file #26694)

Dan Sebald <sebald>
Thu 04 Oct 2012 05:10:54 AM UTC, comment #2: 

I noticed that with the changes _terminal and _main_window aren't used anywhere inside file_editor class.  The updated patch removes those variables and cleans up the class and inherited constructors accordingly.

(file #26693)

Dan Sebald <sebald>
Wed 03 Oct 2012 11:05:13 AM UTC, comment #1: 

I forgot to login for the previous submission.  I'm attaching this message so that I get email about further activity.

Dan Sebald <sebald>
Wed 03 Oct 2012 10:42:40 AM UTC, original submission:  

Here is a patch that shuffles around the code associated with the GUI/IDE editor.  It is meant to make the existing code more in line with the Qt signal/slot, object-oriented paradigm.  For the most part, upward casts of pointers are gone, replaced by signals and slots.

The most important movement of code is to put some of the file-save function inside file editor where it has better access to all of the file names so it can check for any conflicts.  For example, a file save can be initiated in the file editor, thereby signaling the appropriate file editor tab.  The file editor tab will do some of the work with message boxes and file dialogs, then signal back to the editor that it should verify the name doesn't conflict with any existing open file tab.  If there is no conflict, the file editor, in turn, signals back to the file editor tab that a save should be completed.  So, there is some ping-pong-like communication, but if one thinks about this carefully it should make sense.

The manner in which the editor communicates with the file editor tabs (instead of casting the lower level QObject pointer) is via a slot connection with all the file editor tabs.  The editor passes the current active object, and if the file_editor_tab in the slot matches the ID to its value of "this" the file editor will process the action.  This is a fundamental difference in the use of the pointer, i.e., ostensibly a ID tag.  As evidence that the pointer can be used as an identifying tag in the Qt framework, consider this example Qt gives:

 QMessageBox msgBox;
 QPushButton *connectButton = msgBox.addButton(tr("Connect"), QMessageBox::ActionRole);
 QPushButton *abortButton = msgBox.addButton(QMessageBox::Abort);

 msgBox.exec();

 if (msgBox.clickedButton() == connectButton) {
     // connect
 } else if (msgBox.clickedButton() == abortButton) {
     // abort
 }

The above is very analogous to the Qt tab container class widget being used as an identifier tag.  connectButton and abortButton are pointers, but used simply as unique ID tags.

In the Qt signal/slot paradigm there is no need for lower hierarchy objects to retain information about their parent or container.  Therefore, the pointer _file_editor has been removed from file_editor_tab.  Instead, the file editor sets up all the connections for the file_editor_tab.

I've changed all file and message dialogs to WindowModal or NonModal (except the startup wizard).  This works better, in my opinion.  To do this means making signal/slot connections on the fly, and even conditional connections of a signal/slot.  (It's these sorts of techniques that obviate the need for convoluted programming with state variables and return values.)  I know there are going to be some rough edges to smooth out, but the framework is easily modified to fix bugs.  I know how to fix these sorts of things, but I wanted to pause before going forward so others can absorb these changes.  Sometimes others see a creative way of doing things that simplifies code.  We'll open any future bugs to the discussion list.  (E.g., one item I can think of is that we need to come up with a way for Exit to convey the close signals to the editor and file editor tabs.)

Always be diligent of memory leaks.  That is, if one creates a dialog box, QFileDialog, etc. for which the pointer is stored, remember to delete it in the destructor.  If the pointer isn't stored, be sure to set the widget attribute to Qt::WA_DeleteOnClose, i.e.,

      msgBox->setAttribute (Qt::WA_DeleteOnClose);

The find feature is now NonModal so it is possible to edit the text inside the file editor tab and leave the find dialog window open.  That's nice to have.  One thing though: each file editor tab has its own find dialog which is toggled visible/non-visible according to whether it is active.  I suppose the natural thing is for there to be one find dialog for the whole editor, but unfortunately I don't see a way to change the _edit_area of an existing find_dialog.  The solution would be simply to delete the find dialog box upon close--easy change.  However, it would be nice to keep search info (if only the Qt find dialog were a bit more robust).  I've set up the find-dialog behavior in a way that might seem peculiar at first, and it takes some getting used to.  I'm curious about user feedback, as people might like it.  One thing I like about the behavior is that find dialogs are reappearing in the same location they were moved to previously.  I know I myself don't like in editors when the find dialog keeps popping up in some location that blocks a good portion of the screen.  It always seems like I spend so much time moving the find dialog out of the way.

More to do after this patch, but it should be a good start for fixing bugs (the patch already solves several bugs) and programming behavior as desired without too much strain.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #27147:  octave-gui_no_casts-2012dec23.patch added by sebald (77KiB - text/x-patch - Attempt to undo some hunks that altered floating widget window somehow.)
file #26858:  dan.patch added by jordigh (75KiB - text/x-diff)
file #26686:  octave-gui_no_casts-2012oct03.patch added by None (73KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-12-25 ttl StatusIn Progress Done
        Open/ClosedOpen Closed
    2012-12-23 sebald Attached File- Added octave-gui_no_casts-2012dec23.patch, #27147
    2012-11-29 ttl StatusNone In Progress
        Assigned toNone ttl
    2012-11-06 sebald Attached File- Added octave-gui_no_casts-2012nov06.patch, #26880
    2012-11-05 sebald Attached File- Added octave-gui_no_casts-2012nov05.patch, #26876
    2012-10-31 sebald Attached File- Added octave-gui_no_casts-2012oct31_4pm.patch, #26863
    2012-10-31 sebald Attached File- Added octave-gui_no_casts-2012oct31.patch, #26862
    2012-10-30 jordigh Attached File- Added dan.patch, #26858
    2012-10-29 sebald Attached File- Added octave-gui_no_casts-2012oct29.patch, #26854
    2012-10-04 sebald Attached File- Added octave-gui_no_casts-2012oct04_01am.patch, #26694
    2012-10-04 sebald Attached File- Added octave-gui_no_casts-2012oct04.patch, #26693
    2012-10-03 None Attached File- Added octave-gui_no_casts-2012oct03.patch, #26686

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code