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

Upload files and ckeditor #31

Open
wants to merge 15 commits into
base: development
Choose a base branch
from

Conversation

vitstradal
Copy link

second service, now based on branch development.

@Vanuan
Copy link
Collaborator

Vanuan commented Mar 5, 2013

I quickly reviewed your pull request (lot of changes!). It would be much easier to review if you split these into several pull requests: file upload, ckeditor, new page, raw view, list pages, list files.

class CkeditorToGollumWikis < ActiveRecord::Migration
def self.up
add_column :gollum_wikis, :use_ckeditor, :boolean, :default => 0
add_column :gollum_wikis, :store_as_wiki, :boolean, :default => 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this column is for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:store_as_wiki -- full name should be :edit_html_store_wiki, when you use ckeditor it edits HTML.
So if you want store files in wiki format AND use ckeditor it is necessary to convert wiki to html (easy) and after edit convert html to wiki.
If you don't care about store format and html is ok, then it is better to store ckedited documents in HTML.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. This will only work for markdown. AFAIK, other formats don't support html.

@Vanuan
Copy link
Collaborator

Vanuan commented Mar 5, 2013

As for a new page functionality:

I see it in another way. It would be better to reuse and extend edit page: make it possible to name/rename pages on edit page. Then sidebar will contain a link to edit a page with empty name. Of course, there should be a warning if the page already exist...

@Vanuan
Copy link
Collaborator

Vanuan commented Mar 5, 2013

Upload file functionality:

It would be better to have this form where it makes sense - on edit page.

@Vanuan
Copy link
Collaborator

Vanuan commented Mar 5, 2013

Raw view:

would be glad to merge if it was a separate commit without ckeditor dependency.

List pages and files:

I would merge list pages, but without list files. Files are not necessarily images.

@vitstradal
Copy link
Author

As for a new page functionality:
I see it in another way. It would be better to reuse and extend edit
page: make it possible to name/rename pages on edit page. Then
sidebar will contain a link to edit a page with empty name. Of
course, there should be a warning if the page already exist...

It sounds reasonable. I'll think about it. :-)

@vitstradal
Copy link
Author

Upload file functionality:
It would be better to have this form where it makes sense - on edit page.

But there is problem: when you upload file in middle of editing you lost your changes. Solution could be set form target to _blank (or 'upload') to open response in new window.

I solve it by adding sidebar to edit page, so you can open upload page in new window anyway.

In ckeditor, there is tab to upload image, and it fill up image creation form automatically.

@vitstradal
Copy link
Author

Raw view:
would be glad to merge if it was a separate commit without ckeditor dependency.

Ok, but note that raw view have no point when you dont use ckeditor, you can simply edit page. (But yes, difference is, that possibly you have permission to read pages but not edit them).

@Vanuan
Copy link
Collaborator

Vanuan commented Mar 6, 2013

If ckeditor supports only html, I'm afraid I cannot merge these changes. Without conversion back and forth from html to markup, It poses a major vulnerability threat. And that conversion is a really ugly hack. The whole point of gollum and markup languages is that you can safely edit wiki with any text editor of your choice. Syntax highlighting and live preview would be great. But WYSIWYG is a weird & ugly concept, IMO.

I'm thinking more towards integrating this livepreview implementation: https://github.com/bootstraponline/livepreview

@vitstradal
Copy link
Author

If ckeditor supports only html, I'm afraid I cannot merge these changes.

Ok. I respect it.

Without conversion back and forth from html to markup, It poses a major vulnerability threat.

I don't get it. What is difference: if ckeditor can do conversion itself, it will be the same, but conversion will be on client side. Threats are the same or bigger (IMO).

And that conversion is a really ugly hack.

I agree (but not uglier than most of wiki-to-html renders in Github::Markup). I wanted something functional as start point to future step-by-step improvements.

The whole point of gollum and markup languages is that you can safely edit wiki with any text editor of your choice. Syntax highlighting and live preview would be great.
But WYSIWYG is a weird & ugly concept, IMO.

Yes, you correctly write 'IMO'. Other can have other opinion. That is why I make it optional. With 'use_ckeditor' set to no, and 'store_in_wiki' set to no, redmine_gullom is the same as ogrinal version.

@Vanuan
Copy link
Collaborator

Vanuan commented Mar 6, 2013

I don't get it. What is difference: if ckeditor can do conversion itself, it will be the same, but conversion will be on client side. Threats are the same or bigger (IMO).

You're right. I missed the fact that gollum still sanitizes html.

With 'use_ckeditor' set to no, and 'store_in_wiki' set to no, redmine_gullom is the same as ogrinal version.

Well, those additional columns is the major reason I don't want such a change. Since it's only the user's decision, making it a setting per wiki is kind of weird. Why not to create additional form parameters? Say, create a ckeditor tab with checkboxes 'store_as_inline_html/convert_to_markdown'. These can be saved in cookies.

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