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

Change code viewer from RichText to Scintilla #16

Merged
merged 1 commit into from
Feb 25, 2015

Conversation

shashClp
Copy link
Contributor

Using Scintilla wxWidgets widget, I changed the code viewer from parsing code and setting up a RichText widget, to letting Scintilla parse it and format it. The advantages are:

  1. "Better" parser, easily tweakable
  2. Line numbers (in the margin)
  3. Per line timings in the margin
  4. Less code :)

What could be improved:

  1. Line numbers are always active. Could be an option

@CyberShadow
Copy link
Member

Awesome! This was on my list for a while.

Could you please update the build instructions?

Is keywords.txt still needed? I'd think a syntax highlighter would have its own keyword list.

@shashClp
Copy link
Contributor Author

Hey :)

The build instructions remain unchanged, it's using the same wxWidgets version detailed there.

The keywords.txt is still needed, the parser needs a list (or several, if you want) of keywords. The sample provided in wxWidgets uses an inline list for it's example, maybe it would be more interesting to use that, I can check it out if you want to :)

@CyberShadow
Copy link
Member

Hmm, I thought Scintilla was an additional build dependency. All the better then.

@CyberShadow
Copy link
Member

Could you please rebase to squash whitespace and merge commits?

Sleepy's codebase is inconsistent with whitespace, but newer additions have used tabs, so I added an .editorconfig file to indicate that.

A few more nits:

  • StringList::StringList is almost identical to StringSet::StringSet, DRY potential. Could be refactored to use one class with a template or flag, or subclasses.
  • When clicking on a function or call stack item, the respective line ought to be selected (or somehow otherwise highlighted).
  • Would be nice to select the lexer based on the extension. You can use Very Sleepy with any compiled language that emits debugging info in supported formats (e.g. D and probably Go and Rust).

Let me know if you want to handle these, or we can merge and I'll fix these myself.

@CyberShadow
Copy link
Member

When clicking on a function or call stack item, the respective line ought to be selected (or somehow otherwise highlighted).

Actually never mind, the old version behaves the same. Would be nice to have though.

@shashClp
Copy link
Contributor Author

I'll rebase, once I find how (I'm very new to git). About the rest:

  1. I know, I'll think of a cleaner way to handle it, it's a bit crappy as it is
  2. Easy to do, in theory
  3. I'll refactor the code a bit more to make it easier to do, but won't add other languages support for now, as I can't test it easily :/

@CyberShadow
Copy link
Member

Sounds good to me.

One way to squash everything into one commit is to do an interactive rebase (git rebase -i origin/master), and then replacing all but the first "pick" with "fixup".

@shashClp
Copy link
Contributor Author

I'm currently using SourceTree, so I'll check how to do it there, and do the changes we discussed. But that'll be tomorrow :)

@shashClp
Copy link
Contributor Author

About #2, there's a problem. Scintilla only highlights current line if the window has focus (once it loses focus, the highlight is lost). I don't really like windows stealing focus for the sake of it, not to mention that in this case it makes impossible to navigate the function list with the cursors. So I don't think it's a good idea :/

@CyberShadow
Copy link
Member

OK, I'll think about it.

@shashClp
Copy link
Contributor Author

Ok, everything done, only the rebase missing, which I'm battling atm

@CyberShadow
Copy link
Member

There's some inconsistent whitespace, BTW. If you use a recent version of VS, you can get an extension which will configure the editor according to .editorconfig.

@CyberShadow
Copy link
Member

SourceTree should make rebasing very simple:

http://blogs.atlassian.com/2014/06/interactive-rebase-sourcetree/

See squashing (you want to squash all commits into one).

@shashClp
Copy link
Contributor Author

Yeah, I found that post yesterday, but messed up the squash a few minutes ago and had to restart. I'll check the editorconfig extension (or change the spaces to tabs, as I work with spaces).

@shashClp
Copy link
Contributor Author

I think I'm having problems because there's a merge from master in the middle of the stream.

  1. "git rebase -i origin/master" does nothing (doesn't show any commits to fixup/squash/pick)
  2. I can merge until the merge from master with "git rebase -i HEAD~7"
  3. Sourcetree fails to squash, from what it seems to be line conflicts

Spent +2h trying to rebase all my commits to no avail, I might try it tomorrow, or then think what I want to do with this

@CyberShadow
Copy link
Member

  1. "git rebase -i origin/master" does nothing (doesn't show any commits to fixup/squash/pick)

This works for me.

What is "origin", this repository or your fork?

Is the remote up-to-date? (git fetch origin)

@CyberShadow
Copy link
Member

I could pick this up from here if you like, but I'll rewrite your commits on your behalf.

@shashClp
Copy link
Contributor Author

I don't know what is origin, I just wrote that: "git rebase -i origin/master". Do I have to change 'origin' for something else? The remote seem to be up to date.

No, I would prefer to actually make it myself, but already spent 4x the time to try to squash than I actually spent writing the code, so it's getting a bit frustrating :(

@CyberShadow
Copy link
Member

I don't know what is origin,

"origin" is the default name of the remote when you first clone a git repo. Which repo did you clone, https://github.com/VerySleepy/verysleepy or your fork at https://github.com/shashClp/verysleepy ?

@shashClp
Copy link
Contributor Author

I cloned my fork

@CyberShadow
Copy link
Member

OK, so you need to rebase on top of our master:

git remote add upstream https://github.com/VerySleepy/verysleepy
git fetch upstream
git rebase -i upstream/master

@shashClp
Copy link
Contributor Author

Thanks, that did the trick, I could do the rebase after 2 merges :D. Sorry to bother you, first time using git :/

So, it's correct now?

@CyberShadow
Copy link
Member

I don't see anything new. Did you push the new branch?

git push --force

Edit: Also, no worries.

@shashClp
Copy link
Contributor Author

Yes I did, pushed the changes :S

@CyberShadow
Copy link
Member

Oh, I see. It's not rebasing all commits because you've since merged your branch with master.

Here's what I suggest doing:

  1. Clone your fork again to a second directory.
  2. Copy over all the source files, just not the hidden .git directory.
  3. Record all changes as one commit.
  4. Force-push that one commit to your master branch.

@shashClp
Copy link
Contributor Author

But I'm guessing I'd have to clone from the point I started working, because if I clone now, my head will point to the latest commit, thus no changes can be committed, neither pushed :S

If I have to resort to that, I'll probably just start over again, and:

  1. Keep local copies of the 11 commits I did
  2. Close this pull request
  3. Delete my fork
  4. Create new fork
  5. Commit (again) my 11 commits
  6. Rebase to squash those 11 commits to 1
  7. Create new pull request

I know steps 5-6 could be avoided by just using the latest files, but I want to go through the process to learn how to do it :)

@CyberShadow
Copy link
Member

Oh, right, because you used the master branch of your fork. (Usually people create a new branch per pull request.) You'd also want to reset your master to the main repository's master.

Here's a simpler way:

# Add the upstream repository. You might have done this already
git remote add upstream https://github.com/VerySleepy/verysleepy
git fetch upstream

# Reset HEAD and index to the upstream master, but keep working copy changes.
git reset upstream/master

# Commit
git commit -a -m "Changed source viewer to scintilla, percentages and line numbers on margin, code clean-up"

# Push
git push -f origin master

@shashClp
Copy link
Contributor Author

Well, done.

Couldn't try my steps because your instructions seem to have crushed my historic (which I wanted to do through rebase), so I couldn't try it :/

@CyberShadow
Copy link
Member

If you really want to, you can fish your old commits from the git reflog.

What's up with the three-space indent, though?

@shashClp
Copy link
Contributor Author

Where? Just checked the 4 commited files, and could not see a single occurrence of that :S

@CyberShadow
Copy link
Member

Ah, sorry about that. Had to temporarily use another browser, and Chrome treats tabs differently.

CyberShadow added a commit that referenced this pull request Feb 25, 2015
Change code viewer from RichText to Scintilla
@CyberShadow CyberShadow merged commit a692f9f into VerySleepy:master Feb 25, 2015
@CyberShadow
Copy link
Member

What name should I credit you with in the changelog/about dialog?

@shashClp
Copy link
Contributor Author

"Bernat Muñoz Garcia"

Thanks :)

@shashClp
Copy link
Contributor Author

Btw, just rebuild it from source, and when you updated wine, dbghelpw.dll wasn't added (my guess it was been prebuilt and added before), so you might want to fix that :D

@CyberShadow
Copy link
Member

Did you get and build the wine submodule, too?

@CyberShadow
Copy link
Member

I suppose I need to update the build instructions...

@shashClp
Copy link
Contributor Author

Yeah, it's a matter of building it, but it would be nice to add it to the build instructions :)

@CyberShadow
Copy link
Member

Pushed.

@shashClp
Copy link
Contributor Author

Amazing, thanks!

@CyberShadow
Copy link
Member

When clicking on a function or call stack item, the respective line ought to be selected (or somehow otherwise highlighted).

a0207ed

@shashClp
Copy link
Contributor Author

shashClp commented Mar 1, 2015

Oh, cool, it doesn't have the same problems SetCaretLineVisible has :)

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

Successfully merging this pull request may close these issues.

2 participants