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

974 in-browser document editor #1009

Closed
wants to merge 4 commits into from
Closed

Conversation

paulsputer
Copy link
Collaborator

Documents listed on the docs page can now be modified and committed via the browser if the user has write permissions.

@gitblit I've squashed into three commits for the main stages, let me know if there's any other changes to make and if you'd rather me squash to one commit. I would like to get npm downloading automatically with maven/ant but didn't have much luck with that~

@gitblit
Copy link
Collaborator

gitblit commented Jan 25, 2016

@paulsputer Can you rebase on master?

+ npm required as part of the customized build procedure for shimming
+ Ant script added for BuildUI which performs npm commands
+ Imported submodules:
	+ simplemde
	+ codemirror
+ npm build script ensures only a single common CodeMirror definition
+ Update font-awesome to 4.5.0
	+ fixes header icon display for SimpleMDE
+ css z-index adjustments for full-screen editor mode
+ Initial form submit script and commit dialog in place
Factoring out common JGit code from BranchTicketService for usage with doc page commits
	+ getTreeEntries
	+ commitIndex
@paulsputer
Copy link
Collaborator Author

Just rebased and tested @gitblit

@gitblit
Copy link
Collaborator

gitblit commented Jan 26, 2016

I think we are missing some pieces here. gitblit-editor.min.css and gitblit-editor.min.js?
And can you describe the Ant workflow? Is that a once-in-a-while operation or once-after-a-submodule-update ?

@paulsputer
Copy link
Collaborator Author

Those files are generated by the buildUI ant task which will need to be run after a submodule update and then it seems an F5/refresh on the project directory in eclipse is required. This buildUI step needs npm (https://nodejs.org/en/) to be installed, so I wasn't sure whether to join it with one of the other build tasks.

I had hoped to automate the npm install part by using (https://github.com/eirslett/frontend-maven-plugin) but couldn't figure out the moxie settings to get maven to pull it down.

Noticed one ant task does do some js minifying but i'm not aware of a way to get it to do any shimming hence the need for npm which calls the build task defined in package.json.

@gitblit
Copy link
Collaborator

gitblit commented Jan 26, 2016

Moxie != Maven so we can't get that working.

Maybe the problem is my NPM is too old? I'm not into Node nor the JS toolchains. I am running Ubuntu 14.04.

 [exec] npm http 404 https://registry.npmjs.org/codemirror-spell-checker
 [exec] npm ERR! TypeError: Object.keys called on non-object
 [exec] npm ERR!     at Function.keys (native)
 [exec] npm ERR!     at installTargetsError (/usr/share/npm/lib/cache.js:708:24)
 [exec] npm ERR!     at /usr/share/npm/lib/cache.js:638:10
 [exec] npm ERR!     at saved (/usr/share/npm/node_modules/npm-registry-client/lib/get.js:142:7)
 [exec] npm ERR!     at /usr/lib/nodejs/graceful-fs/polyfills.js:133:7
 [exec] npm ERR!     at Object.oncomplete (fs.js:107:15)
 [exec] npm ERR! If you need help, you may report this log at:
 [exec] npm ERR!     <http://github.com/isaacs/npm/issues>
 [exec] npm ERR! or email it to:
 [exec] npm ERR!     <npm-@googlegroups.com>
 [exec] 
 [exec] npm ERR! System Linux 3.13.0-24-generic
 [exec] npm ERR! command "/usr/bin/nodejs" "/usr/bin/npm" "install"
 [exec] npm ERR! cwd /home/james/git/gitblit/src/main/js/simplemde
 [exec] npm ERR! node -v v0.10.25
 [exec] npm ERR! npm -v 1.3.10
 [exec] npm ERR! type called_on_non_object

Would it be a good idea to add the output of the NPM process to the repo? If/when the submodules are updated and files are changed then we could commit those changes. I recognize that frontend devs like NPM, Grunt, Gulp, yada, yada, yada but adding a hard dependency on another build system makes it more difficult for initial build setup.

@paulsputer
Copy link
Collaborator Author

Moxie != Maven so we can't get that working.

Ahh that's why.. ok, I hadn't realized that, I'm not that familiar with it - I thought moxie was a convenience tool for generating the maven build files.

Maybe the problem is my NPM is too old?

It looks that way, you're on 1.3.10 and I believe some of the dependencies require at least version 3. I updated to 3.3.12 when developing this.

Would it be a good idea to add the output of the NPM process to the repo?

Yea that will probably be easier for most development works. I'll add another commit including those files then.

+ Update frontend submodules to latest release:
	+ Codemirror - 5.11.0
	+ SimpleMDE - 1.10.0
@gitblit
Copy link
Collaborator

gitblit commented Jan 30, 2016

@paulsputer the editing feature is really cool & very impressive. I'd like to maintain the old doc page, though, and have a link to the editing page or mode. I do not want it to jump right in to edit mode for an authenticated user.

I can also imagine people will want this extended to editing any file, not just a markup file like Markdown.

What are your thoughts?

@paulsputer
Copy link
Collaborator Author

Thanks @gitblit, ok bit of a long one so grab a coffee... :)

I'd like to maintain the old doc page, though, and have a link to the editing page or mode. I do not want it to jump right in to edit mode for an authenticated user.

Yeah, I was considering to add edit to the blame | history | raw section at the top of the doc page but thought it may appear inconsistent if not shown as an option on the doc listing pages, which then looked to be a much larger change. Hence the approach of: is the user authorized to edit, if not then display raw mode.

Happy to add the edit in as it does provide a good clear separation for users intending only the read the document. A few questions:

  1. Do we want a completely separate page & uri for the editor?
  2. Should we also display an edit option as we do a view on the doc listing?
  3. To avoid any inconsistencies with two different markdown renderers, would we rather render the view mode on the client? i.e. SimpleMDE configured without buttons (or perhaps just the full screen reading mode button, although then it almost begs for an edit button too if the user has edit permissions).

I can also imagine people will want this extended to editing any file, not just a markup file like Markdown.

Certainly. This PR puts the client side plumbing in place ready for it by providing a single version of CodeMirror. Other file types would display the CodeMirror interface (with syntax highlighting etc) rather than the SimpleMDE one so will probably be a good PR on it's own. The doc editor commit logic currently follows that of the branchTicketService which looks to overwrite HEAD but I'm pretty sure we will want to call one of the JGit merge strategies for other files.

Also I can imagine another PR will be for SimpleMDE interface when creating ticket comments.

@paulsputer
Copy link
Collaborator Author

@gitblit I'll close this PR for the moment as I've found ProseMirror (https://github.com/prosemirror/prosemirror) which, as I'm looking into it further, seems to be a more appropriate fit for Gitblit and supports a wider range of document formats and use cases.

It's from the same author as CodeMirror yet has no dependency on it, so greatly simplifies the client-side build script.

With this in mind, having a dedicated editFile page appears a better approach to addressing edit capabilities.

Hopefully won't be long for me to rework some of these changes into a new PR to support this. :)

@paulsputer paulsputer closed this Feb 4, 2016
@gitblit
Copy link
Collaborator

gitblit commented Feb 4, 2016

Ok, sounds good. Sorry, I'm easily distracted by too many other active things and it's difficult sometimes to get back to the review.

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.

None yet

2 participants