Goto line dialog

[leutwe]

No description provided.

[j6t]

The commits have your cryptic github email address as author. Do you insist in using that, or would I be allowed to use your full name and a more useful email address instead?

[leutwe]

I have changed the github setting.
Where could I see this cryptic email address?

[j6t]

Type git show on the command line to see your most recent commit. Look at the Author: line.

[j6t]

I'll take this commit, but I'll update the commit message. Do allow that?

[leutwe]

Do what you think it is the best.

[j6t]

This Github review system is pure shit. I attempted to comment on 1159675, but it tacked the comment onto the overall conversation.

Do what you think it is the best.

Are you willing to update the commits and adjust them to the project requirements? For example, you should read through existing commits to get to know the commit message style. In particular, there is a summary line, and a justification of the change, i.e., an explanation why the change is needed.

This includes the goal that the commit wants to achieve. Look, for example, at the commit Preferences Back Timeout: 0 has now special meaning. It must explain in the commit message, at best already in the summary line, what the special meaning is.

Also, don't do fixup changes; squash the fixups into the earlier change that introduced the breakage. This concerns the tab conversion fix.

Note also that the indentation style is quite weird. Indentation is 4 positions, but tab width is still 8 and tab characters are used instead of 8 consecutive spaces.

[leutwe]

I'm not so familiar with git.

Do you mean I should make a fresh branch, and replay the commits?
Do you prefer to have no tabs in the code?

[j6t]

Given the current state, it is probably easiest to make a new branch and rebuild the commits. But, please, when you are finished, do not make a new pull request, but update this one if possible.

You can also use git rebase -i origin to fix up the previous commits.

You should use tabs in the code, but only when you have two or more indentation levels, because one indentation level (4 positions) is not enough for a tab. Then 3 levels is one tab and 4 spaces, etc.

[j6t]

Thank you for the update.

I like the idea to offer the option that the debugger never goes into the background. But that special value should not be 0, but "infinite". Can we have an edit box with a special value "never"?

I won't take the last commit. I doubt that there is any performance to gain by turning off RTTI. Besides that, the warning that this would remove is more important than to permit custom builds with special compiler options.

Regarding the Goto Line dialog: Good idea. I have a patch sitting around that moves the Find dialog to a toolbar instead of having it as a floating dialog. It would be great to have the Goto line entry in a similar way. But that can possibly be changed later.

[j6t]

Please mimic the coding style that you see elsewhere in this project: No space after the opening parenthesis and before the closing parenthesis. Do not use the _Ci, which looks like a type indicator. Do not use this->. Do not copy the member variable unnecessarily, it does not make the code easier to read and reason about.

[j6t]

If we can have a special user-visible value "never", then we do not need an explanation. The special value can just be a very large value, say 1'000'000ms, then we do not even need special code for the timer, it just takes 16 minutes until the debugger goes into the background. That should be sufficient for "never".

If you do want to add an explanation, you can drop the bit about "raise it", because that happens also when the timeout is not the special value.

[leutwe]

I like that the window is raised if the program stops. To have a timer running for a feature the user didn't like have, I don't know. I often like to see at least a part of the kdbg window, to can activate it through a mouse click instead of search it at the taskbar, because some other windows which are much more seldom used are in front of it. Instead of 0, it would also be possible to take a negative value, which is seeing it as unsigned int is a big value. The idea behind 0 was also the short description what is possible through that value,.
Do you really prefer a ComboBox LineEdit combination for that? The user expectation could be, that he likes see the previous used values too.

[j6t]

I think that raise-to-top happens always even when there is a timeout. (At least that is the intent and I think it worked when I implemented it some decades ago.) Therefore, in this regard your new setting is not special. It's only special because it does not send KDbg into the background.

Whether it is a Combobox or something else I do not care much. I only saw some controls where it was possible to specify numbers and in addition one special, named value. I also would not mind to make -1 the special value for your new behavior, but in the user-interface it should be this one named special value that I propose to be "never".

[leutwe]

I thought about it. What do you think if we replace the QLabel with a QCheckbox that it looks like this:
[x] lower window after [ms]: __200
If the checkbox is unchecked m_backTimeout is set to the negative value of checked m_backTimeout) minus 1.
In this way no addition configuration value would be needed.

[j6t]

The checkbox is fine, too.

Concerning the configuration value, we have to be careful and keep in mind that the setting can be read by old versions of KDbg. Will they do something useful with a negative timeout, or will they be confused? I think that a new configuration value for the checkbox is better.

[leutwe]

Regarding the Goto Line dialog: Good idea. I have a patch sitting around that moves the Find dialog to a toolbar instead of having it as a floating dialog. It would be great to have the Goto line entry in a similar way. But that can possibly be changed later.

I would also prefer this - but I did not get this to work.

[j6t]

Thank you!

I like the mechanics of the Goto dialog. The implementation needs some polishing, because it does not follow the project style. I would like to point out the details, but the review function on Github are completely incompatible with the way I would like to review commits. (In particular, my comments on an old commit remain even after the code was updated by a later commit, and then the comment is totally outdated.)

• Do not invent your own style to name variables.
• Do not open a block unnecessarily.
• Do not comment on trivial lines of code.
• Please do not write if ( foo ), write if (foo).
• Do not change code that does not need to be changed.

Can we please have a cue text in the edit box of the Goto line dialog, like "Line number". Because on my screen the dialog title is so short that it only says "Goto...".
Can it be avoided that there is an own top-level menu that reads "Goto" with a single sub-entry "Goto line number"? I would like to have the "Goto line number" entry in the Edit menu, or perhaps in the View menu.

Can you please rebuild the commits so that it appears that there were no errors made from the beginning? You would use git rebase -i to do that. I mean, do not have "oops, this fixes an earlier error"-commits.

If you think you do not want to do that exercise, I can do it.

And, BTW, do you want to remain anonymous regarding your first name and last name, i.e., do you prefer not to have your name etched into the history permanently? Or would you reveal your name?

[leutwe]

I would prefer if you do the changes you want. in the way you want. I think this will shorten things as not so much recursions are needed.

Regarding the menu entry, my knowledge how the Kde things are working is to low, I didn't get this changed.

[j6t]

I've found a problem with the Goto Line dialog: It should skip disassembly lines, but does not. Can you have a look? Class SourceWindows has lineToRow() that may be helpful.

I have pushed a preliminary clean-up of your work. You can download it with git fetch origin; the branch is origin/goto-line.

[leutwe]

I just saw it today.
I will take a look at it.

[leutwe]

Here is a patch which should solve the issue:

#git diff
diff --git a/kdbg/sourcewnd.cpp b/kdbg/sourcewnd.cpp
index 9bc0630..b89554c 100644
--- a/kdbg/sourcewnd.cpp
+++ b/kdbg/sourcewnd.cpp
@@ -346,10 +346,11 @@ void SourceWindow::gotoLine( const QString& text)
	 if (!isSuc)
		return;
 
+       int row = lineToRow(lineGoto);  /*< if assembly is visible row != line. */
	 QTextCursor cursor(document());
	 cursor.setPosition(0);     // goto file first line
 
-    isSuc = cursor.movePosition(QTextCursor::Down, QTextCursor::MoveAnchor, lineGoto - 1);
+    isSuc = cursor.movePosition(QTextCursor::Down, QTextCursor::MoveAnchor, row - 1);
 
	 setTextCursor( cursor );
 }

[j6t]

Thank you for the update. Please give me some time to consolidate the patches.

[leutwe]

Through the change regarding assembly lines for the goto feature, now a assert happens if the specified line number is greater than the line number present in the file.

I added the following at void SourceWindow::gotoLine( const QString& text):

if ( m_rowToLine.size() == 0 )
{
        return;
}
if ( size_t( lineGoto_i ) >= m_rowToLine.size() ) lineGoto_i = m_rowToLine.size() - 1;

before
QTextDocument* document_po = document();

But maybe a change of
int SourceWindow::lineToRow(int line)
would be better.

[j6t]

Please write a new commit with the suggested changes (those from above and these most recent suggested changes and update the pull request. Thanks.

[leutwe]

Hopefully I done the git related things in a usable/understandable way.

[j6t]

Thanks. Will have a look.

[j6t]

I've just pushed out an update of my branch goto-dialog with your latest change squashed in. Do you want to make the changes mentioned in the commit title, i.e.:

• add an OK button to the dialog
• move the menu entry into the View menu (because it does not make sense to have a new top-level menu that has a single entry, even though that would be some standard or convention to have a "Go" menu)