List SQL History for a selected table
The only comment I would give for the spec would be that instead of a dialog I think a special component or maybe just a
mode in the IDE would work better. If a component then it can be made into a layer API or an SPI where components in the
view may be registered, or if just a mode then anything may be placed in it. Then the user can use it as a dialog, dock
it, or what ever they like. As a database user I would prefer that over a dialog (especially a modal one) which I have
to bring up and down as when I'm actually working with SQL I will most likely be working with it more than anything
until done, and often use history features.
Here are my comments looking at the code currently checked in:
DVC01 - would it make more sense to have a method saveSQL(url, sql) and let this method construct the SQLHistory object?
DVC02 - the method saveSQL() and the method save() are confusing - they seem similar in name. I think saveSQL() should
probably be called addSQLToList() or addSQL() or something.
DVC03 - statements that execute successfully are not saved if *any* statement in the file fails. That doesn't seem
right. This makes me wonder if saveSQL() should just go ahead and write the statement to the XML file and you get rid
of the save() method. Otherwise you are having to deal with delayed writes and the associated risks.
DVC04 - What is the value of SQLHistoryController? It looks like it's not being used...
DVC05 - I am guessing the motivation for separating SQLHistoryManager from SQLHistoryPersistenceManager is to allow for
multiple ways to persist the SQL. I'm not sure all this abstraction is needed - YAGNI (You Ain't Gonna Need It). You
could just do everytihng in the SQLHistoryManager.
DVC06 - SQLHistoryPersistenceManager.create() is very confusing. Shouldn't this be called writeSQL() or something?
DVC07 - If you really want to abstract persistence, then SQLHistoryPersistenceManager should be creating the FileObject,
DVC08 - It seems expensive to look up the DataFolder and the FileObject each time you write history. Why not get the
folder once and reuse it? e.g. in the constructor for SQLHistoryPersistenceManager, do
FileSystem historyFS = DataFolder.findFolder(fo).getPrimaryFile().getFileSystem()
then in the "create" method (shd be called "save") you just do
DVC09 - I can't find any uses of SQLHistoryModel or SQLHistoryUrlObserver either - how are these used?
DVC10 - In your tests, you don't validate that the statements actually get saved. Shouldn't you read the XML file and
make sure it all looks kosher?
DVC11 - I don't see any UI yet, I guess that's coming?
Good feedback, thanks. I'll address them soon.
Most features are in. I'll continue to work on the issues I listed below.
Can open the dialog from a new SQL Editor toolbar icon that lists the SQL executed.
Matchbox is mostly working but needs some improvements.
Insert code should be working.
Remaining features to add:
Limit the number of SQL statements to save.
Add a tooltip to the Table and Connection dropdown
Some bugs need to be fixed:
The first SQL statement saved is not appearing the first time opening the dialog (all SQL appear subsequent times
opening the dialog)
May be some mismatch in the SQL and corresponding date that appear in the Table.
Connection dropdown was working but I just broke it while trying to fix the selection of All Connections
I think this is supposed to be a P2
SQL history data should be accurrate now.
Dropdown is working better
Tooltips are there
But I broke a couple of things:
- bogus results appear when entering sql that doesn't match
- selection in the table is not working
Selecting row in table is not working -
Entering text that doesn't match any sql presents bogus data
(I'll file a P1s for these)
Verified with build 080618.