This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 91280 - new modul localhistory - review
Summary: new modul localhistory - review
Status: RESOLVED FIXED
Alias: None
Product: versioncontrol
Classification: Unclassified
Component: Localhistory (show other bugs)
Version: 6.x
Hardware: All All
: P1 blocker (vote)
Assignee: issues@versioncontrol
URL:
Keywords: API_REVIEW_FAST
Depends on: 92671
Blocks:
  Show dependency tree
 
Reported: 2006-12-19 14:44 UTC by Tomas Stupka
Modified: 2007-02-15 09:51 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Architecture Answers for Local History module (71.93 KB, text/plain)
2006-12-19 15:34 UTC, Tomas Stupka
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Stupka 2006-12-19 14:44:39 UTC
i'd like to ask for a review for a planed local history module.
Comment 1 Tomas Stupka 2006-12-19 15:32:30 UTC
The module tracks changes made on files in the IDE and provides an UI to:
- view previously saved versions from a file 
- to revert a file to some from its older versions

for more information see the answered architecture questions in branch localhistory 

versioncontrol/localhistory/arch.xml


Comment 2 Tomas Stupka 2006-12-19 15:34:33 UTC
Created attachment 36803 [details]
Architecture Answers for Local History module
Comment 3 Jaroslav Tulach 2006-12-21 15:02:38 UTC
Y01 Few times the document says, that there is no API. Usually we consider 
something some other module can depend on being an API, so there is an API. It 
is the location of the local storage, preferences, etc. So I would say "There 
is no stable API"

Y02 One of the open questions is where to store the storage. I would say it 
should be under $userdir (which means that NetBeans 5.5 and NetBeans 6.0 are 
not going to share this storage), this similifies the versioning I guess. The 
storage shall not be $userdir/config as that is an area to be accessed by 
FileObjects while the storage is supposed to be handled directly by 
java.io.File access. I would personally prefer $userdir/cache/localstorage.

Y03 Please use <api group="java.io.File" .../> to describe the location and 
format of the storage. Please make sure that one of the main storage files 
contains some version string, so you can detect that the cache is of old 
format and delete it in future if needed.

Y04 There is a note about using lookup for own actions. Do you subclass some 
SystemAction subclass or do you implement your own actions. Can you give me 
links to source files?

Y05 Contracts imported from versioning module should be marked as <api 
type="import" category="friend" .../> as you are probably a friend of 
versioning.

Y06 There were some notes about performance (especially doing cleanup of the 
cache on startup), that might be interesting to Radim & co.

Y07 Imho you should have a TCR (a P2 bug) reported against versioning module 
to switch to new contract provided by masterfs that will allow you to be 
notified "before file change". Imho masterfs owner shall agree that he 
implements such contract (a P2 for masterfs).

Otherwise ok and I am looking forward to see this in the build I use daily.
Comment 4 Tomas Stupka 2006-12-21 15:50:52 UTC
Y04: 
actions used for the context menu in explorer are NodeAction subclasses, 
the remaining ones, used in the localhistoryview are SystemActions

see in branch localhistory, project folder versioncontrol/localhistory/

all actions are in package org.netbeans.modules.localhistory.ui.actions

the mentioned lookup is created in 
org.netbeans.modules.localhistory.LocalHistoryVCSAnnotator

since yesterday there are a few more usages of lookups also in 

org.netbeans.modules.localhistory.ui.view.LocalHistoryView
org.netbeans.modules.localhistory.ui.view.StoreEntryNode


Y06: 
i must admint i'm not sure if the cleanup on startup is the best idea.
it's still open when it should be triggered (startup, on demand, ...). 
However, i agree that localhistory should be a candidate for Radim & co. 
Comment 5 rmatous 2006-12-21 16:45:35 UTC
Y07 I can't promise notification before file change, just can notify you that
OutputStream was issued.
Comment 6 Tomas Stupka 2007-01-02 10:57:43 UTC
my previous commit - typo in Y04:

> the remaining ones, used in the localhistoryview are SystemActions
  the remaining ones, used in the localhistoryview are _no SystemActions_
Comment 7 Jaroslav Tulach 2007-01-03 09:01:40 UTC
Re. actions:

Actually I found two that subclass NodeAction, like
http://www.netbeans.org/source/browse/versioncontrol/localhistory/src/org/netbeans/modules/localhistory/ui/actions/Attic/ShowLocalHistoryAction.java?rev=1.1.2.4&only_with_tag=localhistory&view=markup
and two that directly subclass AbstractAction, like
http://www.netbeans.org/source/browse/versioncontrol/localhistory/src/org/netbeans/modules/localhistory/ui/actions/Attic/DeleteAction.java?rev=1.1.2.4&only_with_tag=localhistory&view=markup

I guess the use of NodeAction is ok. I suggest you register the action into a 
systemfilesystem folder like Actions/Versioning/LocalHistory/ or so. As a 
result it would be possible for users to put those actions into toolbar, 
assign shortcuts to them etc.

However this causes a problem with your non-NodeActions. They should implement 
ContextAwareAction and find the files they need to operate on from Lookup. 
I'll send you a sample code.
Comment 8 _ rkubacki 2007-01-03 09:22:57 UTC
re startup impact: I guess that every startup will be affected a bit but if we
can manage it reasonably it can be OK. Let's measure how long it is. I do not
expect large amount of data like in
http://www.netbeans.org/issues/show_bug.cgi?id=91082. 
Comment 9 Peter Pis 2007-01-09 08:33:22 UTC
Reassigning issue to "localhistory" component.
Comment 10 Tomas Stupka 2007-01-18 13:45:20 UTC
Y01 - Y05 - changed in arch.xml and code
Y07 - issue #92671, issue #92676
Comment 11 Tomas Stupka 2007-02-15 09:51:13 UTC
fixed