Closed Bug 88208 Opened 24 years ago Closed 23 years ago

UI for publishing (publish/site settings dialogs)

Categories

(Core :: DOM: Editor, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: Brade, Assigned: cmanske)

References

Details

Attachments

(1 file, 19 obsolete files)

40.96 KB, patch
Brade
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
This bug is intended to cover the design and implementation of a publish dialog for Composer. I am assigning this bug to mpt for his design effort. mpt--please reassign back to me for implementation (unless you also want to implement the dialog).
Blocks: 28792
Brade, what options does (or will, by the time the dialog is implemented) the Composer back end support for publishing? In particular, does it have the ability to: 1. Upload files (e.g. images) which are included/embedded in the current page? 2. Upload all files in a user-chosen folder? 3. Include or exclude subfolders when uploading a folder? 4. Detect which local files are newer than their remote equivalents, and only upload those which have changed? And a final question: Composer 4.x's Publish dialog includes a field for entering the page title -- whereas the publish UI for other editors, such as FrontPage and Dreamweaver, do not. Was there any particular reason for this? (Perhaps people were unwilling to enter a title before a file went onto the Web where a title would matter; but it also makes the Publish dialog more complicated.)
I have made an attachment to BUG #28792 for this, as I did not know of this bugs existence at the time :) Let me know if you want it reattached here. Perhaps this bug is not needed, and everything can be tracked in 28792.
Attached image proposed UI for publish (obsolete) —
Attached image server settings for publishing (obsolete) —
brade, do you need these mock-ups implemented?
Time to start on this, so I'll take the bug. Brian what time do you have to help with this?
Assignee: mpt → cmanske
Whiteboard: EDITORBASE (5 weeks)
Target Milestone: --- → mozilla1.0
Charley, I'm ready. Give me a spec and I'll start right away.
The xul looks good, a couple things (probably done before my landing): - Why do EditorPublish.xul and EditorPublishOverlay.xul need dialogOverlay.xul and dialogOverlay.js now that they're <dialog/>s? (By the way, the overlay brings in the js, so any editor dialog that does this is trying to load the js twice...) - <dialog/>s don't need <keyset id="dialogKeys"/> anymore - The default orientation for <tabpanels/> is vertical, so it doesn't need to be specified - <text/> is deprecated (it should have been removed, is it still working?) - The event handler should be "onselect", not "onSelect" - The style for MoreFewer button should be in a css file - The default orientation for <groupbox/> is vertical, so it doesn't need to be specified - Does EditorPublishOverlay.xul need the html namespace on its <overlay/> tag? In other words (since there obviously aren't any html tags in the dialog), does EditorPublish.dtd use <html:br/> like some of the other dialogs? Doesn't look like it. - What is an onselect on a box supposed to do...?
Thanks for the review! All suggested changes have been made. "dialogOverlay" includes as well as other older XUL cruft has been removed or converted to new sytax. The "onselect" on the hbox was leftover from an earlier version. I've removed it. The reason for the style on "MoreFewer" is that in all the other uses in Composer dialogs, the text is "More Properties / Fewer Properties", which certainly isn't correct here. As an interim hack, I've use "More" and "Less" as the button text, and put the style in the XUL since the button should be smaller than the "properties" version, which does set its width in CSS. This is a 'first draft' checkin and details like that will be fixed as it matures. I'll attach a new patch after working out other details from brade's review.
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0 → mozilla0.9.7
Fixes old xul found by Blake. Don't force input of page title suggested by Brade.
Attachment #57618 - Attachment is obsolete: true
Keywords: patch, review
Whiteboard: EDITORBASE (5 weeks) → EDITORBASE (5 weeks) need r=,sr=
"First draft" is ready to checkin
Whiteboard: EDITORBASE (5 weeks) need r=,sr= → EDITORBASE (5 weeks), FIX IN HAND, need r=,sr=
Comment on attachment 57732 [details] [diff] [review] Update to incorporate reviewers suggestions. You still have a few <text/> uses in that patch. Could you please fix those? Other than that, sr=hewitt
Attachment #57732 - Flags: superreview+
All <text are removed (Forgot about the EditorPublishOverlay.xul)
Comment on attachment 57732 [details] [diff] [review] Update to incorporate reviewers suggestions. r=brade
Attachment #57732 - Flags: review+
First draft is checked in. Whoopy! Keeping bug open for discussion and improvements of this UI.
Keywords: patch, review
Whiteboard: EDITORBASE (5 weeks), FIX IN HAND, need r=,sr= → EDITORBASE (5 weeks)
Charley, can you summarize what you checked in (what's in and what's missing)? I see the new UI but it appears to be non-functional. What type of feedback (specifically) are you looking for with this check-in? Thanks.
You can use the "Publish" or "Publish As" commands to bring up the Publish dialog. There you can create new site profiles that should be retained in prefs, so nexttime you launch you should see them. You can also play with site settings in dialog launched by "Edit | Publishing Site Settings...". Again, the interface should work for creating and modifying site settings. It is not hooked up to Brade's core publishing code yet, so you can't actually publish anything. Comments on the UI are welcome. Missing in the UI are the "other files" support (i.e., images, css etc) in the "More" section of the Publish dialog, as well as other setting suggested in the original spec.
The dialog resizes quite a lot when switching between the "publish" and "settings" tabs, causing the mouse-cursor to not any longer being located over the smaller of these dialogs. This in turn frequently causes me to "loose" the dialog, since I "raise windows mouseover". The dialog vanishes behind some other window more often than not. (Sawfish WM, linux) The same phenomena might occur on Windows using the unsupported "X-mouse" from "TweakUI". Why not leave the advanced publishing options present at all times? That way the dialog would have the same size regardless of chosen tab. Otherwise, the dialog should resize based on a fixed upper left window position.
Work for 0.9.7 milestone is done. Re: comments in #19: I don't see the problems you describe in Windows builds. Switching from "Publish" to "Settings" simply expands the height of dialog dialog and doesn't move the upper-left corner position. Is this a problem only in Linux? As to the "mouseover" problem, we are now following strict window parenting rules for dialogs. I use Tweak UI, and have used the "raise windows mouseover" feature in the past, but I don't see that option under my current Windows 2000 installation, so I couldn't test it.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
New featues are not "EDITORBASE".
Priority: -- → P2
Whiteboard: EDITORBASE (5 weeks) → (5 weeks)
Blocks: 112984
moving milestone
Target Milestone: mozilla0.9.8 → mozilla0.9.9
To be clear, this bug is intended to cover these items: * publish panel - basic info - more/fewer views - directory info * site settings other publishing-related UI stuff will be covered in a new bug.
Summary: UI for publishing → UI for publishing (publish/site settings dialogs)
Depends on: 118719
*** Bug 110450 has been marked as a duplicate of this bug. ***
Keywords: nsbeta1+
Attached patch Next round of UI work (obsolete) — Splinter Review
This implements improvements in the main Publish dialog, Settings Dialog, getting and setting data in prefs, and in Publish/Publish As, and Save commands so they use the publishing database to find existing publishing sites.
Attachment #50525 - Attachment is obsolete: true
Attachment #50526 - Attachment is obsolete: true
Attachment #57732 - Attachment is obsolete: true
Comment on attachment 68586 [details] [diff] [review] Next round of UI work this patch needs work; Charley and I discussed many issues on AIM including: * lack of try/catch in many places * moving save functions to end of ComposerCommands.js * FinishHTMLSource() should be moved in cmd_publish * addressing some issues now instead of adding comments * adding overall comments for how things work * .toLowerCase() isn't required in some places anymore (GetScheme does it for us) * fix JS warnings including this assignment: publishData.siteName == publishData.PublishUrl; Is it possible to split this patch up into separate pieces?
Attachment #68586 - Flags: needs-work+
Attached patch Updated patch (obsolete) — Splinter Review
This implements saving and getting user password from Password Manager and should finish latest work in publishprefs.js
Attachment #68586 - Attachment is obsolete: true
Attached patch Updated patch (obsolete) — Splinter Review
More changes per reviewers comments
Attachment #68869 - Attachment is obsolete: true
The only changes relative to last patch are in publishprefs.js: SetPublishString() method added and nsISupportsWString is used as pref type rather than "char".
Attachment #68962 - Attachment is obsolete: true
Depends on: 125128
Attached patch Updated patch (obsolete) — Splinter Review
more changes after review discussions.
Attachment #69047 - Attachment is obsolete: true
Comment on attachment 69257 [details] [diff] [review] Updated patch * be sure to leave all of these set to false: -const gShowDebugOutputStateChange = false; * I don't understand why we enable the Publish command if IsUrlAboutBlank; shouldn't we leave it disabled so users will choose Publish As... ? * move this line closer to where it is used or place it with the global debug output flags (in that vicinity): + const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener; * move this line to be closer to where it is used also (inside gShowDebugOutputStateChange?) + var requestSpec; * I think these lines should be swapped so that the assignment is inside the try: + url = uri.spec; + try { * Is there some reason to keep this as separate functions: StripUsernamePassword StripUsernamePasswordFromURI * do we need a try/catch around this line: + return prefsService.getBranch("editor.publish."); * if this line fails, shouldn't catch return false? passwordManager.addUser(publishData.publishUrl... * I don't understand why this line is in ValidateData(). Can it be removed or is it used later in the function (not in diff)? + var title = TrimString(gDialog.PageTitleInput.value); r=brade on these particular files: editor.xul EditorPublish.xul EditorPublishOverlay.xul EditorPublishSettings.xul EditorPublish.dtd
Attachment #69257 - Flags: needs-work+
Concerning "why we enable the Publish command if IsUrlAboutBlank;" We do that for "Save" as well. It simply lets them attempt to save or publish a new file even if now changes were made. They will end up in the Publish As dialog anyway. For consistency with how "Save" works, I think this is corrrect. Concerning moving "const nsIWebProgressListener =" and "var requestSpec;" they should be at the top and will be used by final code once the debug output block is moved. (We will need to use requestSpec even outside the debugging code.) I changed the debug output anyway in upcomming patch, so it makes sense in that context. The rest of the suggestions are fixed in next patch.
Attached patch Update (obsolete) — Splinter Review
The only things changed are in ComposerCommands.js and moving the "url = uri.spec" line in editorUtilities.js, as suggested.
Attachment #69257 - Attachment is obsolete: true
Comment on attachment 69305 [details] [diff] [review] Update r=brade with these changes: * be sure all flags are disabled like: +const gShowDebugOutputStateChange = true; * remove some extra spaces * remove this line: + var title = TrimString(gDialog.PageTitleInput.value);
Attachment #69305 - Flags: review+
Comment on attachment 69305 [details] [diff] [review] Update This has been checked in.
Attachment #69305 - Attachment is obsolete: true
Attached patch Next round (obsolete) — Splinter Review
Implemented in this patch: 1. Separate Save and Publish commands (Save is only "local", Pubish is remote) Add support for setting initial site site when launching Publish dialog. 2. Added Publish toolbar button (see bug 125128 for icons and CSS patches) 3. Implement "Publish to" submenu under File menu and toolbar button. 4. Improve Publishing Settings dialog to support passwords, use <listbox> an fix various bugs. 5. More factoring and improvements in publish prefs methods for smarter "one click publishing" support.
Keywords: patch, review
Whiteboard: (5 weeks) → (5 weeks) FIX IN HAND, need r=,sr=
Attached patch Next round -- update (obsolete) — Splinter Review
Addresses brade's review comments: 1. Added "SetElementHidden()" utility so we check if element exists when hiding things. 2. Backed out new "DoAfterSave()" method -- to many conditional differences to factor into common method (in ComposerCommands.js). 3. Small spelling error changes
Attachment #70022 - Attachment is obsolete: true
Depends on: 126272
Depends on: 126450
Breaking down patch for clarity. These changes are the same as those in "Next round--update" (http://bugzilla.mozilla.org/attachment.cgi?id=70199&action=view) for publishprefs.js
Attachment #70199 - Attachment is obsolete: true
Breaking down patch for clarity. These changes are the same as those in "Next round--update" (http://bugzilla.mozilla.org/attachment.cgi?id=70199&action=view) for dialog-related files
Breaking down patch for clarity. Further changes for Publish commands and UI: 1. Publish button on toolbar does not have a submenu of publish sites. We want this button to enable / disable as the Save button does to show current modified state of document, and the submenu is inconsistent with that action. 2. Reduce number of items in File menu to: Publish [the "one click publish" command, same as on toolbar] Publish to > a submenu with: Create or Change Publish Info... [launch publish dialog ] ----------------- SiteName 1 SiteName 2 etc. for all the sites we have prefs for. (the wording for the "Create..." menuitem is under discussion!) Note that is the old "Publish As" action. In the Publish dialog that is launched, we allow setting Page Title, filename, and site settings, and can create a new site, but it is not as powerful as the full "Publish Settings" dialog accessed by "Edit | Publish Site Settings" menu.
Comment on attachment 70335 [details] [diff] [review] Improvements to Publishing dialogs r=brade
Attachment #70335 - Flags: review+
Kin and I found a problem in the RemoveSite() method in EditorPublishSettings.js: New code is: // Remove one item from site data array gPublishSiteData.splice(index, 1); // Remove item from site list gDialog.SiteList.selectedIndex = -1; gDialog.SiteList.removeItemAt(index); // Check if we removed last item and reselect a site if (index >= gPublishSiteData.length) index--; InitSiteSettings(index);
Comment on attachment 70335 [details] [diff] [review] Improvements to Publishing dialogs sr=kin@netscape.com
Attachment #70335 - Flags: superreview+
Comment on attachment 70342 [details] [diff] [review] Improvements to publishing commands, menuitems, and add toolbar support r=brade assuming we don't change some of the strings (we should wait for RobinF to review all of them rather than changing them now)
Attachment #70342 - Flags: review+
Per review request, I added this to ValidateData() method in EditorPublish.js to change sitename in list when user changed it in Settings Tab: // Update selected item if sitename changed var selectedItem = gDialog.SiteList.selectedItem; if (selectedItem && selectedItem.getAttribute("label") != siteName) { selectedItem.setAttribute("label", siteName); gDialog.SiteList.setAttribute("label", siteName); }
Comment on attachment 70342 [details] [diff] [review] Improvements to publishing commands, menuitems, and add toolbar support sr=kin@netscape.com Should SetSaveAndPublishUI() be moved out of editor.js since it is shared with mail?
Attachment #70342 - Flags: superreview+
Attached patch Update to publish prefs code (obsolete) — Splinter Review
Fixes GetSiteList() to correctly put default sitename first if requested. After much thinking about brade's comments about GetMatchingUrlLength() method and its usage, I can't come up with a different method that really helps! Instead I changed the name and added lots of comments to help explain the logic.
Attachment #70333 - Attachment is obsolete: true
Comment on attachment 70389 [details] [diff] [review] Update to publish prefs code r=brade though I'm still disappointed about using length for matching sites; also dependent on new bugs filed to cover these issues: * handling of multiple identical prefs (different logins or other reasonable variation) * handle case where user is editing url with user@ in it
Attachment #70389 - Flags: review+
Comment on attachment 70389 [details] [diff] [review] Update to publish prefs code sr=kin@netscape.com
Attachment #70389 - Flags: superreview+
Depends on: 124561
Depends on: 121314
Depends on: 126784
Work or this milestone done. Changing to next to continue.
Target Milestone: mozilla0.9.9 → mozilla1.0
Depends on: 127084
Depends on: 127465
*** Bug 126272 has been marked as a duplicate of this bug. ***
Depends on: 128058
Whiteboard: (5 weeks) FIX IN HAND, need r=,sr= → (5 weeks) FIX IN HAND reviewed
Includes fixes to bug 126784 and bug 127084
Attachment #70335 - Attachment is obsolete: true
Attachment #70342 - Attachment is obsolete: true
Attachment #70389 - Attachment is obsolete: true
Comment on attachment 71899 [details] [diff] [review] New bug fixes and changes to UI from discussions Issues addressed in this patch: 1. Remove "Pulish to" menu and simplify "Publish As" command code 2. Change some wording and UI for the "Other directory" part of Publish Dialog 3. Add code to deal with username and/or password embedded in a publishing URL 4. Fix problems updating settings in the Publish Settings dialog 5. Fix problems detailed in bug 126784 and bug 127084
Attached patch Update for New bug Fixes... (obsolete) — Splinter Review
A few changes after reviewer comments
Attachment #71899 - Attachment is obsolete: true
Whiteboard: (5 weeks) FIX IN HAND reviewed → (5 weeks) FIX IN HAND, need r=,sr=
Attached patch Updated patch (obsolete) — Splinter Review
Change relative to last patch is in editorUtilities.js: I eliminated previous StripUsernamePassword(), replacing it with modified version of GetAndStripUsernamePassword() in which the usernameObj and passwordObj out param objects are optional. All calls to "GetAndStripUsernamePassword" were changed to "StripUsernamePassword"
Attachment #71955 - Attachment is obsolete: true
More small changes per reviewer comments
Attachment #72665 - Attachment is obsolete: true
Comment on attachment 72679 [details] [diff] [review] Update (last, I hope?) r=brade
Attachment #72679 - Flags: review+
Whiteboard: (5 weeks) FIX IN HAND, need r=,sr= → (5 weeks) FIX IN HAND, need sr=
Attachment #72679 - Flags: superreview+
Whiteboard: (5 weeks) FIX IN HAND, need sr= → (5 weeks) FIX IN HAND, reviewed
Comment on attachment 72679 [details] [diff] [review] Update (last, I hope?) a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72679 - Flags: approval+
No longer depends on: 127084
Blocks: 127084
checked in. All new problems found in publishing UI should be filed as separate bugs.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: (5 weeks) FIX IN HAND, reviewed
looks good. excellent work Charley. verified in 3/8 trunk build on windows if anyone finds any new issues, file them as separate bugs.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: