Closed
Bug 495492
Opened 15 years ago
Closed 15 years ago
Phase 2 of the editor communication spec
Categories
(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect)
addons.mozilla.org Graveyard
Admin/Editor Tools
Tracking
(Not tracked)
RESOLVED
FIXED
5.0.8
People
(Reporter: clouserw, Assigned: smccammon)
References
Details
Attachments
(3 files, 3 obsolete files)
217.14 KB,
patch
|
clouserw
:
review+
|
Details | Diff | Splinter Review |
7.04 KB,
text/plain
|
clouserw
:
review+
|
Details |
30.78 KB,
patch
|
clouserw
:
review+
|
Details | Diff | Splinter Review |
Implement phase 2 of the internal editor communication spec: http://docs.google.com/Doc?id=dcm3h3vx_9gdn2bwhg&hl=en
Public viewable version of the Google Doc http://docs.google.com/View?id=dcm3h3vx_9gdn2bwhg
Assignee | ||
Comment 2•15 years ago
|
||
Rey, In the current spec, the details in 1.3 are nearly identical to 1.2. Is there supposed to be more for enhanced replying functionality?
It's meant to clearly outline the functionality expected when creating a new comment or replying to an existing comment. So if you can re-use code, that's just icing on the cake.
Assignee | ||
Comment 4•15 years ago
|
||
A few implementation comments and questions mostly regarding attachments. If anything seems odd or could be better designed, let me know. URLs and Storage: - The editors controller should serve attachments only to authorized users via URL: editors/attachment/<attachment_id> - Uploaded attachments should be stored in a restricted directory: webroot/attachments/versioncomments/<comment_id>/<filename>. Would a directory outside of webroot be preferred? - Is there anything special needed in order to mirror uploaded files to webheads or are they all sharing a single remora installation via nfs or equivalent? Attachment DB Schema: - table attachments * id (primary key) * versioncomment_id * filename * mimetype * size * description * created * modified - Along with description, mimetype and size are to be displayed with the comment. These could of course be determined on the fly, but a thread with 100 attachments would then do that many extra file opens per request - yuck. Non-attachment related: - Should comment subscription management be handled by the users controller (it would fit well in users/edit), or by the editors controller (editors/subscriptions ?) - It's not currently part of the spec, but a preview function would seem to me a major requirement for any composition allowing markup. New bug?
Assignee: nobody → smccammon
Status: NEW → ASSIGNED
Reporter | ||
Comment 5•15 years ago
|
||
(In reply to comment #4) > (a bunch of stuff about attachments) Rey, is spending time on attachments in comments the best use of our time? We're planning on removing the sandbox which I expect will lower comment traffic, and with the proliferation of free online sharing sites and most editor/devs with their own web space there just doesn't seem like a lot of demand for this. It's a substantial investment in resources (dev time, maintenance, storage+backups) and I want to make sure it's worth it. I know you've talked to editors about how to improve our system and we've been making a lot of changes in that direction - has there been a lot of outcry for the ability to add attachments? > Non-attachment related: > > - Should comment subscription management be handled by the users controller > (it would fit well in users/edit), or by the editors controller > (editors/subscriptions ?) Either is fine with me. We've already got a convenient "Notifications" tab when you edit your profile. If we could figure out a way to make this not take up a ridiculous amount of room that would be a decent spot. > - It's not currently part of the spec, but a preview function would seem to me > a major requirement for any composition allowing markup. New bug? That's not a bad idea. You could do a live preview with JS. I use live-comment-preview on my blog comments.
> Rey, is spending time on attachments in comments the best use of our time?
I suspect not. You can table the requirement for the immediate future and if it's something that I think needs to be reconsidered later, we can discuss it accordingly.
Assignee | ||
Comment 7•15 years ago
|
||
The editors comment system includes syntax parsers for HTML/XML, CSS, JS, SQL, Diff/Patch and plain text. Any languages that I missed that can be used by add-on developers?
Attachment #388788 -
Flags: review?(clouserw)
(In reply to comment #7) > Created an attachment (id=388788) [details] > Fenced Code Blocks + Syntax Highlighting > > The editors comment system includes syntax parsers for HTML/XML, CSS, JS, SQL, > Diff/Patch and plain text. Any languages that I missed that can be used by > add-on developers? XUL? It's similar enough to JS though so it may not be an issue.
Assignee | ||
Comment 9•15 years ago
|
||
> XUL? It's similar enough to JS though so it may not be an issue. http://mccammos.khan.mozilla.org/comm-p2-syntax-49592/site/en-US/editors/review/60816#editorComment475 Indeed. The out-of-the-box JS parser/highlighter seems to work fine. I can change the "Javascript" dropdown to "Javascript / XUL" in case it isn't obvious.
Assignee | ||
Comment 10•15 years ago
|
||
This patch depends on the markdown patch in bug 495491.
Attachment #389229 -
Flags: review?(clouserw)
Assignee | ||
Comment 11•15 years ago
|
||
Since Markitup has some basic preview hooks, I thought I'd try it out. This works fine, though snagging sessionCheck from another form so that Markitup's post will pass the CSRF check feels kind of hackish. I can think of a couple of cleaner solutions, but this was the least amount of code. (depends on Fenced Code Blocks + Syntax Highlighting patch)
Attachment #389503 -
Flags: review?(clouserw)
Reporter | ||
Updated•15 years ago
|
Attachment #389229 -
Flags: review?(clouserw) → review-
Reporter | ||
Comment 12•15 years ago
|
||
Comment on attachment 389229 [details] [diff] [review] Subscription Management Most of this is fine, just a couple things: components/editors.php: Nice catch on making sure a user is still an editor before sending mail. However, removing the restrictions on findAllById() makes cake go crazy and pull in a huge amount of data for each user (every approval, add-on, collection, subscription, etc.). The best way to do this would be to make a getUsers() (you can model after getAddons() - it's just a loop) and getUser() to find what you need. _thread_subscribe_unsubscribe(): Similar to above, Addon->findById() is going to pull in a huge amount of data. Use getAddon() with only the association you need (see the function declaration or examples).
Assignee | ||
Comment 13•15 years ago
|
||
Updated syntax highlighting patch to work with latest trunk (and to use the new webroot/vendors area).
Attachment #388788 -
Attachment is obsolete: true
Attachment #390513 -
Flags: review?(clouserw)
Attachment #388788 -
Flags: review?(clouserw)
Reporter | ||
Updated•15 years ago
|
Attachment #390513 -
Flags: review?(clouserw) → review+
Assignee | ||
Comment 14•15 years ago
|
||
Syntax highlighting in r30370.
Assignee | ||
Comment 15•15 years ago
|
||
Patch updated to latest trunk.
Attachment #389503 -
Attachment is obsolete: true
Attachment #390592 -
Flags: review?(clouserw)
Attachment #389503 -
Flags: review?(clouserw)
Comment 16•15 years ago
|
||
Available on preview Scott?
Reporter | ||
Comment 17•15 years ago
|
||
Comment on attachment 390592 [details]
Markdown/Markitup Preview v2
A preview button that isn't next to the post button makes me sad. MarkDown--
Attachment #390592 -
Attachment mime type: application/octet-stream → text/plain
Attachment #390592 -
Flags: review?(clouserw) → review+
Assignee | ||
Comment 18•15 years ago
|
||
(In reply to comment #17) Markdown preview added in r30476. > A preview button that isn't next to the post button makes me sad. MarkDown-- We can add a button there easily enough - the editor really isn't doing anything special (unless one enables its live preview function). An enhancement for 5.0.9? Rey - all but subscription management should be available on preview.a.m.o. I'll have that last patch fixed up shortly.
Assignee | ||
Comment 19•15 years ago
|
||
Attachment #389229 -
Attachment is obsolete: true
Attachment #390847 -
Flags: review?(clouserw)
Reporter | ||
Comment 20•15 years ago
|
||
Comment on attachment 390847 [details] [diff] [review] Subscription Management v2 This is awesome. I suspect the management of it will get overwhelmed with threads though, but I don't see a great way around that. Maybe not making the <div> scroll would be a good start, but even then you're looking at a huge page. Anyway, we can think about it.
Attachment #390847 -
Flags: review?(clouserw) → review+
Assignee | ||
Comment 21•15 years ago
|
||
Subscription management added in r30557.
Updated•15 years ago
|
Keywords: push-needed
Updated•8 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•