Community
Participate
Working Groups
Created attachment 140835 [details] Add support for "Publish on build event" option in server editor Currently the user has two options: 1) Publish on any resource change 2) Never autopublish I believe a third option, publish on build events, would make a lot of sense and be appreciated by our users. (It was, in fact, a request by a user of ours). I've made a patch for it. There is a note, though, that currently Bug 282482 might prevent my patch from working. However if that bug is fixed, my patch should work quite well (assuming you agree with the internal api changes (which should be safe as I've added methods, not removed any). I would like to see this make it into 3.2 if possible. I also encourage you to vote on the blocking bug above if it turns out it is indeed a bug and I'm not just insane.
It might not be ideal but isn't this similar to saying 1 sec auto-publishing? I think the purpose of allowing the user to modify the interval of the autopublish thread was to allow this kind of granular customization. Every user is different some wouldn't want to publish changes immediately, others do. To satisfy such a broad audience and also to further allow each server adopter to provide their own specific time interval, the current code seems appropriate.
Hi Rob. The patch appears to be incomplete - it has the UI changes but no implementation in server.core. :) Could you please post/add a new patch or explain what the underlying changes would be? I think this is an interesting enhancement, assuming of course that the platform bug doesn't cause problems.
Created attachment 141038 [details] Fixed patch Forgot the core portion, sorry
The new patch uses the suggested workaround from the referenced bug and actually works. The only issue now is whether you accept this patch and the new methods. Have a look when you can ;)
> I think the purpose of allowing the user to modify the interval of the > autopublish thread was to allow this kind of granular customization. Every user > is different some wouldn't want to publish changes immediately, others do. To > satisfy such a broad audience and also to further allow each server adopter to > provide their own specific time interval, the current code seems appropriate. Some users use their workspace with autobuild turned off, perhaps without asking for a build for 10 or 15 minutes at a time. Then they'll initiate one build, and want a publish to occur. Then maybe they'll switch to some rapid development and initiate builds every 2 minutes and want a publish for each one. If a user sets his autopublish to every 120 seconds, but then goes on a lot of code-diving and small changes, for 20 minutes, it'll publish 10 times in that interval (assuming the user is making small changes). This may be undesirable for the user.... especially if the project is quite large. Conversely, if the user sets it to autopublish every 15 minutes (to account for these long dives) but then goes on a series of rapid change-and-test sessions, autopublish doesn't publish often enough and he has to change the setting again =] Anyway I believe the new patch should work better =] I wouldn't mind if you don't like the core (internal) API additions if you have a better way to accomplish this so feel free to tweak it, but I think it's not bad.
I agree with Rob's assessment and I'd like to say that this feature is extremely welcome. There's no good publish interval that results in seamless code-publish-test every time, thus "zero turnaround" suffers. If the developer has control over when the publish event happens, she is free to complete a unit of work before triggering a compile-and-publish, never having to wait for an unexpected auto-publish.
pinging so this doesn't get lost ;)
I guess no one's had time to look into this patch for me? I know M1 is basically frozen now?
Rob, We will be looking at this for M2, sorry about the delay. Still doing planning and working in 3.1.1.
Do we have a milestone target for this at all? Thanks.
(In reply to comment #10) Not yet, but soon.
Hi Angel: What's been preventing the assimilation of this bug with a patch attached? The patch was added 2 months ago.
I have been focusing on 3.1M and unable to get to review the patch. I initially thought I would have time for M2 because I know of your great interest on this, but I was only able to sit down and think about the scenario and have some chats with people about the different usage and the benefits out of it. As I mentioned before, this is something that I certainly want to get in 3.2. Once I finalize the plan I will be able to have an accurate target for you but until the plan comes out, it is hard for me to give any accurate target. Is there a particular reason for getting this ASAP?
It's not anything critical... it's just that it's a fairly uncontroversial enhancement requested by one of our users and with a patch attached.
From the adoptor's point of view, I think this function is good as well. However, from the patch, it is not clear what the exact behaviour of the new settings supposed to be. The code has: final boolean buildEvent = (kind == IncrementalProjectBuilder.INCREMENTAL_BUILD) || (kind == IncrementalProjectBuilder.FULL_BUILD) || (kind == IncrementalProjectBuilder.AUTO_BUILD && ResourcesPlugin.getWorkspace().isAutoBuilding()); It looks like whether a publish occurs is based on a combination of the enablement of the auto build and the actual build event is triggered by the auto build. Is this flag meant to be for publishing only when a manual build is kicked off or is it supposed to be handling the auto build cases as well. The label of the new GUI seems to suggest it is for both.
The issue here is related to https://bugs.eclipse.org/bugs/show_bug.cgi?id=282482 Basically, even if the autobuilder was turned off, it would still throw a build event (type autobuild), even though a build never happened. Without this logic, checking for a build event basically turns into a resource change event, as all resource change events will have the auto-build flag. So this is a check for (incremental) or (full) or (auto AND auto-builder is enabled). So this should be called / executed whenever an actual build happened. Whether it was an autobuild or a forced build shouldn't matter. This code is trying to avoid non-builds pretending to be builds.
Created attachment 154151 [details] New Patch, runs auto-publish on project close / delete events New Patch, Angel.
For changes on OverviewEditorPart: Suggest to change line 632 from: autoPublishEnableBuild .setSelection(publishSetting == Server.AUTO_PUBLISH_RESOURCE); to: autoPublishEnableBuild .setSelection(publishSetting == Server.AUTO_PUBLISH_BUILD); Suggest to change line 636 from: whs.setHelp(autoPublishEnableBuild, ContextIds.EDITOR_AUTOPUBLISH_ENABLE); to: whs.setHelp(autoPublishEnableBuild, ContextIds.EDITOR_AUTOPUBLISH_BUILD); and add a new context id EDITOR_AUTOPUBLISH_BUILD to the ContextIds. Other than that, the code looks good (I haven't tested to validate).
Elson, Good catch on the ContextIds Rob, Please remember to check spacing and update the copyright on the files. Good patch. Changes committed to HEAD.
Changes released to HEAD
Angel: Could you elaborate or link to policy on updating copyrights, and the spacing? Specifically, just a quick summary of what I should have done would be very helpful. Thanks.
Created attachment 156898 [details] Important fix to smoketest failures There were some NPEs in the case the "event" was null. These occured during run on server, or using the add / remove module page. These have been fixed. However this also (somehow?) revealed a small failure in the new ServersView2, which occasionally tries to refresh a disposed tree.
(In reply to comment #22) Are the changes to the ServerView2 necessary. If we are trying to stabilize the current changes, why are we adding changes to the ServerView2.
(In reply to comment #23) Given that some teams have already smoke test with the patch for both Server and ServerView2, I feel strong about checking in the whole patch. But it would be nicer to understand in more details the scenario and impact of the ServerView2 problem.
Angel: After making the patch for the NPE, I saw repeated error log entries where the view's tree was disposed during either "run on server" or the add / remove modules wizard. I didn't trace through exactly what was causing the refreshServerContent(etc) call, but I did think it was prudent to not refresh if the tree was disposed. Also, not refreshing when the tree is disposed seems very safe to me and can only remove errors, am I right? Regardless, upon adding the change to the view, the error log entries stopped coming. I would vote for adding the entire patch. If you'd rather give it a quick test without the servers view changes, feel free ;) But I was experiencing widget disposed errors consistently.
I committed this patch without the server ui part to minimize the changes. I will open a new bug to commit the server ui part.
Created bug# 301581 to address the refreshing problem
Angel did you ever open the other bug for the UI? This feature is fairly useless without the user being able to set the flag via a radio button or some such method. I do not consider this bug closed without a UI for the user to... use.
Created attachment 164050 [details] new publish option (In reply to comment #28) Rob, I am little lost on what radio button you are referring to?. This feature has been completed for a while already
(In reply to comment #29) > Created an attachment (id=164050) [details] > new publish option > > (In reply to comment #28) > > Rob, I am little lost on what radio button you are referring to?. This feature > has been completed for a while already Yep sorry. I only looked at the bugzilla entry, which said: > I committed this patch without the server ui part to minimize the changes. > I will open a new bug to commit the server ui part. I was unable to find this other entry and searched through bugzilla for a bit.
(In reply to comment #30) That was referring to the UI part in bug# 301581
New Gerrit change created: https://git.eclipse.org/r/108856
