search

andreikop / enki

A text editor for programmers
http://enki-editor.org
GNU General Public License v2.0
161 stars 38 forks source link

Preview sync #147

Closed bjones1 closed 10 years ago

bjones1 commented 10 years ago
andreikop commented 10 years ago

Hi. I tested the code. Incredible! I thought it would be very difficult.

Few issues, which shall be fixed before I can merge it to master and release as next official Enki version:

Coding style

Could you please format your code according to https://github.com/hlamer/enki/wiki/Hacking-guide. (Long lines, separated_with_underscore names)

Tre dependency

Tests

Could you please cover new functionality with tests? I explained in the hacking guide, why it is so important. It would be quite difficult to simulate click on preview, but you can call js_click from a test and check cursor position in the code after it. You also can move cursor from a test code and check selection in the QWebView.

Selection in the QWevView

I tested selection in QWevView on enki/README.md file, and it often looks incorrectly.

It is possible to select whole line, on which cursor is located? If it is impossible to make good selection in 99% of cases, can we just scroll preview, but do not do any selection.

preview-sync-selection

bjones1 commented 10 years ago

Andrei,

Thanks for your feedback. My comments are placed in-line with your e-mail below.

Bryan

On Tue, Dec 10, 2013 at 1:15 AM, Andrei Kopats notifications@github.comwrote:

Hi. I tested the code. Incredible! I thoughthttps://github.com/hlamer/enki/issues/121it would be very difficult.

Few issues, which shall be fixed before I can merge it to master and release as next official Enki version: Coding style

Could you please format your code according to https://github.com/hlamer/enki/wiki/Hacking-guide. (Long lines, separated_with_underscore names)

Sounds good. One exception: will you allow >120 characters for comments? I feel that comments should be word-wrapped by any decent text editor (Enki certainly does); manually inserting line breaks makes it harder to write good comments, since I end up spending time fixing indentation, rather than writing better documentation.

Tre dependency

  • I want to make Enki installation as easy as possible. Not all users need Preview synchronization, so, I don't want to force them to manually compile tre. Could you please make tre dependency optional. If it is missing - just do not do Preview/code sync.

Good point. I'll do that.

  • I release .deb and .rpm packages of Enki. We have to make python-tre .deb and .rpm packages to make sync available for major Linux distribution users.

Unfortunately, I know nothing about packaging on Linux. It looks like someone's already tried for Debian but I don't understand all the details at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=566790. It looks like the RPM packages are done ( http://rpmfind.net/linux/rpm2html/search.php?query=python-tre&submit=Search+...&system=&arch=). Any ideas on how to fix the Debian package? And am I correct in assuming the RPM package works?

Tests

Could you please cover new functionality with tests? I explained in the hacking guide, why it is so important. It would be quite difficult to simulate click on preview, but you can call js_click from a test and check cursor position in the code after it. You also can move cursor from a test code and check selection in the QWebView.

Will do. Yes, I agree that testing in important. I planing on working up a decent set of tests.

Selection in the QWevView

I tested selection in QWevView on enki/README.md file, and it often looks incorrectly.

I agree. It worked better in CodeChat. I'm still working on finding the root cause. I also have an idea for a better algorithm. I'll keep you posted on this.

  • Sometimes selection doesn't match cursor position
  • Selection with range 5 symbols looks not intuitive

It is possible to select whole line, on which cursor is located?

I agree that the selection of a range of characters isn't intuitive. I used to use self.textBrowser.textCursor().select(QtGui.QTextCursor.LineUnderCursor) for a QTextEdit widget, but that's not possible (that I know of) in a QWebView. Especially on a screen with lots of text, showing the user where the cursor is located is very important. Let me see if I can come up with something better.

If it is impossible to make good selection in 99% of cases, can we just scroll preview, but do not do any selection.

[image: preview-sync-selection]https://f.cloud.github.com/assets/419720/1712161/74c45fb6-616a-11e3-992d-92901910096e.png

— Reply to this email directly or view it on GitHubhttps://github.com/hlamer/enki/pull/147#issuecomment-30204274 .

Bryan A. Jones, Ph.D. Associate Professor Department of Electrical and Computer Engineering 231 Simrall / PO Box 9571 Mississippi State University Mississippi state, MS 39762 http://www.ece.msstate.edu/~bjones bjones AT ece DOT msstate DOT edu voice 662-325-3149 fax 662-325-2298

Our Master, Jesus Christ, is on his way. He'll show up right on time, his arrival guaranteed by the Blessed and Undisputed Ruler, High King, High God.

andreikop commented 10 years ago

Hi, Bryan

See my comments below

will you allow >120 characters for comments? I feel that comments should be word-wrapped by any decent text editor (Enki certainly does); manually inserting line breaks makes it harder to write good comments, since I end up spending time fixing indentation, rather than writing better documentation.

Wrapped lines are drawn near the left side of code window. It is difficult to see structure of a indented code. You don't have to break lines on 120 - word length characters, just break on sentence end when length is grater than 60. It doesn't require long time and text doesn't have to be reformatted after every edit.

someone's already tried for Debian but I don't understand all the details at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=566790. It looks like the RPM packages are done ( http://rpmfind.net/linux/rpm2html/search.php?query=python-tre&submit=Search+...&system=&arch=). Any ideas on how to fix the Debian package? And am I correct in assuming the RPM package works?

Enki and Qutepart packages are built and hosted on OBS. You can see list of supported distributions here. It seems like

All distribs ship tre version 0.8. Is it suitable for preview sync?

bjones1 commented 10 years ago

Andrei,

Comments below. Progress thus far: highlighting the entire line works; sync is disabled if TRE's not installed.

Bryan

On Wed, Dec 11, 2013 at 11:52 PM, Andrei Kopats notifications@github.comwrote:

Hi, Bryan

See my comments below

will you allow >120 characters for comments? I feel that comments should be word-wrapped by any decent text editor (Enki certainly does); manually inserting line breaks makes it harder to write good comments, since I end up spending time fixing indentation, rather than writing better documentation.

Wrapped lines are drawn near the left side of code window. It is difficult to see structure of a indented code. You don't have to break lines on 120 - word length characters, just break on sentence end when length is grater than 60. It doesn't require long time and text doesn't have to be reformatted after every edit.

Ouch. Still painful, but I'll do it. Perhaps I'll convert you some day -- I'm a big fan of literate programming, which is what my Enki extensions are aimed at.

someone's already tried for Debian but I don't understand all the details at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=566790. It looks like the RPM packages are done (

http://rpmfind.net/linux/rpm2html/search.php?query=python-tre&submit=Search+...&system=&arch= ). Any ideas on how to fix the Debian package? And am I correct in assuming the RPM package works?

Enki and Qutepart packages are built and hosted on OBShttps://build.opensuse.org/package/show/home:hlamer:enki/enki. You can see list of supported distributions here.

OK, I'll take a look.

It seems like

All distribs ship tre version 0.8. Is it suitable for preview sync?

Yes, that's the latest version from github and what I use. It doesn't build on Windows, so I've submitted a pull request to the author, but this doesn't affect unix.

— Reply to this email directly or view it on GitHubhttps://github.com/hlamer/enki/pull/147#issuecomment-30391111 .

Bryan A. Jones, Ph.D. Associate Professor Department of Electrical and Computer Engineering 231 Simrall / PO Box 9571 Mississippi State University Mississippi state, MS 39762 http://www.ece.msstate.edu/~bjones bjones AT ece DOT msstate DOT edu voice 662-325-3149 fax 662-325-2298

Our Master, Jesus Christ, is on his way. He'll show up right on time, his arrival guaranteed by the Blessed and Undisputed Ruler, High King, High God.

bjones1 commented 10 years ago

Andrei,

My changes are complete. I believe I've addressed all the items you've mentioned, except for creating a python-tre package (which doesn't belong in this pull request, IMHO). Would you review it?

Bryan

andreikop commented 10 years ago

Hi

I tried to use your branch. Looks much better!

Could you please enable issues in your Enki fork settings. Probably you should

So, it will be easier to discuss preview sync in separate issues

bjones1 commented 10 years ago

OK, I think I've turned it on.

bjones1 commented 10 years ago

Andrei,

Just wanted to ping you on this -- I've enabled issues and replied on github, but not sure if that made it to you inbox or now.

Bryan

On Sun, Feb 9, 2014 at 9:28 AM, Andrei Kopats notifications@github.comwrote:

Hi

I tried to use your branch. Looks much better!

Could you please enable issues in your Enki fork settings. Probably you should

So, it will be easier to discuss preview sync in separate issues

Reply to this email directly or view it on GitHubhttps://github.com/hlamer/enki/pull/147#issuecomment-34576690 .

Bryan A. Jones, Ph.D. Associate Professor Department of Electrical and Computer Engineering 231 Simrall / PO Box 9571 Mississippi State University Mississippi state, MS 39762 http://www.ece.msstate.edu/~bjones bjones AT ece DOT msstate DOT edu voice 662-325-3149 fax 662-325-2298

Our Master, Jesus Christ, is on his way. He'll show up right on time, his arrival guaranteed by the Blessed and Undisputed Ruler, High King, High God.

andreikop commented 10 years ago

I opened 5 issues on https://github.com/bjones1/enki/issues Have you seen it?

bjones1 commented 10 years ago

Andrei,

Oops, no. It looks like my github -> email settings aren't correct. Thanks -- I'll take a look!

Bryan

On Thu, Feb 13, 2014 at 2:45 AM, Andrei Kopats notifications@github.comwrote:

I opened 5 issues on https://github.com/bjones1/enki/issues Have you seen it?

Reply to this email directly or view it on GitHubhttps://github.com/hlamer/enki/pull/147#issuecomment-34958481 .

Bryan A. Jones, Ph.D. Associate Professor Department of Electrical and Computer Engineering 231 Simrall / PO Box 9571 Mississippi State University Mississippi state, MS 39762 http://www.ece.msstate.edu/~bjones bjones AT ece DOT msstate DOT edu voice 662-325-3149 fax 662-325-2298

Our Master, Jesus Christ, is on his way. He'll show up right on time, his arrival guaranteed by the Blessed and Undisputed Ruler, High King, High God.

bjones1 commented 10 years ago

Andrei,

FYI, I'm fairly busy with some other deadlines this week. I'll certainly get back to this ASAP. In the meantime, a student I'm advising has found a great approach which should dramatically improve the sync using a largest common subsequence algorithm. More as soon as I can find time!

On Thu, Feb 13, 2014 at 2:46 AM, Bryan A. Jones bjones@ece.msstate.eduwrote:

Andrei,

Oops, no. It looks like my github -> email settings aren't correct. Thanks -- I'll take a look!

Bryan

On Thu, Feb 13, 2014 at 2:45 AM, Andrei Kopats notifications@github.comwrote:

I opened 5 issues on https://github.com/bjones1/enki/issues Have you seen it?

Reply to this email directly or view it on GitHubhttps://github.com/hlamer/enki/pull/147#issuecomment-34958481 .

Bryan A. Jones, Ph.D. Associate Professor Department of Electrical and Computer Engineering 231 Simrall / PO Box 9571 Mississippi State University Mississippi state, MS 39762 http://www.ece.msstate.edu/~bjones bjones AT ece DOT msstate DOT edu voice 662-325-3149 fax 662-325-2298

Our Master, Jesus Christ, is on his way. He'll show up right on time, his arrival guaranteed by the Blessed and Undisputed Ruler, High King, High God.

  • 1 Tim. 6:14b-15 (The Message)

Bryan A. Jones, Ph.D. Associate Professor Department of Electrical and Computer Engineering 231 Simrall / PO Box 9571 Mississippi State University Mississippi state, MS 39762 http://www.ece.msstate.edu/~bjones bjones AT ece DOT msstate DOT edu voice 662-325-3149 fax 662-325-2298

Our Master, Jesus Christ, is on his way. He'll show up right on time, his arrival guaranteed by the Blessed and Undisputed Ruler, High King, High God.

andreikop commented 10 years ago

Hi, Bryan

Thank you for your fixes. I pulled your changes, made some minor fixes and rebased the branch to the current master. It works fine now and passes the tests! My branch preview_sync contains final version. I'll merge it to master when v14.03 is released. (I think this week).

If you are going to do some new changes - fork my preview_sync branch to avoid conflicts and merges, open new pull request.

bjones1 commented 10 years ago

Andrei,

Great, thanks for accepting this feature! I'm excited to have it in Enki.

I'm working on the feature you suggested (center on preview), and will rebase my work to your preview_sync branch. I'll submit a pull request when it's ready.

bjones1 commented 10 years ago

I'd like to re-open this one so I can work on getting the python-tre package ported to Linux variants. I'm stuck and wondered if you could point me in the right direction.

I'm working on getting python-tre working in OpenSuse 13.1, since I think it should be a minor tweak to the Fedora sources. So, I found tre in the OpenSuse build service site at https://build.opensuse.org/package/show/openSUSE:Factory/tre.

  1. Is this the "right" place? I can't find it in the openSUSE:Maintainence area.
  2. I can't find the python-tre package listed in the .spec file for this project. Where do I find that?

If you're available at some point to chat, that would be helpful -- I have a lot of newbie questions on this if you have time.

andreikop commented 10 years ago

Hi

For .deb distributions good starting point is http://packaging.ubuntu.com/html/. I will not give any suse-fedora-rpm links, because .rpm package was done by yajo but not by me. https://build.opensuse.org/project/show/home:hlamer:enki might be useful as an example.

I can't find the python-tre package listed in the .spec file for this project. Where do I find that?

It seems like tre RPM package already exists, but python-tre doesn't. You should create it, and it will depend on tre.

My experience says me, that creating packages is looong job. Could you please close https://github.com/bjones1/enki/issues?page=1&state=open before doing the packages. So I'll be able to release 14.04 with preview but without python-tre?

andreikop commented 10 years ago

... I'm available for help on irc.freenode.net 31.03.2014 20:20 пользователь "bjones1" notifications@github.com написал:

I'd like to re-open this one so I can work on getting the python-tre package ported to Linux variants. I'm stuck and wondered if you could point me in the right direction.

I'm working on getting python-tre working in OpenSuse 13.1, since I think it should be a minor tweak to the Fedora sources. So, I found tre in the OpenSuse build service site at https://build.opensuse.org/package/show/openSUSE:Factory/tre.

Is this the "right" place? I can't find it in the openSUSE:Maintainence area. I can't find the python-tre package listed in the .spec file for this project. Where do I find that?

If you're available at some point to chat, that would be helpful -- I have a lot of newbie questions on this if you have time.

— Reply to this email directly or view it on GitHub.