confirm , send to right developer , set target and change the summary

fs->oj: your dialog ...

.

"According to the OpenOffice.org roadmap
(http://tools.openoffice.org/releases) this issue was retargeted to
OOo Later." 

change subcomponent to 'none'

blocking the meta issue

I modified the issue.I had started from AppController.cxx:Execute ,get the 
selected table's name through getSelectionElementNames(...),then extended some 
place to transfer it to where it was needed.
 

Created attachment 45706 [details]
issue18907 patch

Created attachment 45802 [details]
combined patch for easier readability

grabbing for patch review, since OJ is on vacation

Okay, first: thanks for the patch!
Second: I'll attach my review notes. Items marked with "+" need to be fixed
(nothing serious, relax :), items with "-" are suggestions for changes you could
do to the patch, I'll not insist on them.

Created attachment 45805 [details]
review notes

fs->dyf: sorry, I forgot to re-assign this issue to you, so I think you didn't
notice my review comments above.

hi,fs
:)I'm sorry i didn't notice your review comments above.I have notice I missed 
the patch of OApplicationController.And Your proposal is reasonable,thanks!I 
will complete it soon.

Created attachment 46134 [details]
patch for it

Add patch of AppController.cxx.Modified some places according to Mr Frank's 
proposal.

re-opening. Sorry, an issue is to be set to FIXED as soon as the patch is
committed to CVS, not earlier. See "A Bug's life time" at
http://qa.openoffice.org/issue_handling/index.html.

grabbing for review

fs->dyf: better, but not yet finished:
- With a recent version, this does not compile on some platforms because
  OTableCopyHelper::pasteTable (the version in TableCopyHelper.cxx:507)
  does have a (new) parameter _sTableName, which it does not use. For some
  compilers, this is a warning, and warnings are reported as errors in
  a regular OOo build.

- Now we have the desired behaviour when we paste, for instance, a Calc
  cell range into a Base document. In this case, the table which is selected
  in Base is used as default table name - fine.
  However, now we have an inconsistency: When you copy a Base table itself
  to the clipboard, and then paste it, then the current selection in the
  application is *not* respected.
  We should fix this case, too, for consistency reasons.

- in DExport.cxx:781, you change the default from WIZARD_DEF_DATA to
  WIZARD_APPEND_DATA. That's not completely what I meant with my comment:
  Now the default is *always* to append, even if the suggested table name
  does not yet exist. For instance, select *no* table in Base, and then
  paste => you will get a dialog which has "Append" pre-selected, but
  a non-existent table name. That's not what we want to have here.

  I suggest you completely remove the last parameter of the OCopyTable
  constructor, and make a reasonable guess inside the constructor: If
  the a table with the given name already exists, then pre-select "Append",
  else pre-select "Create".

  (Alternatively, you can spare this piece of the patch, since it's  not
  part of the original bug.)

Thanks.
(side note: If possible, please next time generate one big file containing the
changes for all files, this makes applying and reviewing on my side much easier.
diff -ru should do. Thanks.)

ok.

I don't know why do this judgment In OCopyTableWizard::OCopyTableWizard? 

if ( m_xSourceConnection == m_xConnection )
            {
                Reference<XTablesSupplier> xSup(m_xConnection,UNO_QUERY_THROW);
                m_sName = ::dbtools::createUniqueName(xSup->getTables
(),m_sSourceName,sal_False);
            }
 else
		m_sName = m_sSourceName;

Looks like a strange heuristics to me, which probably can be removed, and
replaced by setting m_sName to m_sSourceName. In this case, it should be ensured
that "Append" is pre-selected then.

Hi,Frank

I have a problem. ::rtl::OUString getDatabaseName() in 
\dbaccess\source\ui\app\ : OApplicationController, and ::rtl::OUString 
ODataAccessDescriptor::getDataSource(),what is the difference of their return 
value? When equal, ranging from what time?

Both have effectively the same semantics. The different names are for historical
reasons only.

In general, a data source name can either be a "registered named" (see the
com.sun.star.sdb.DatabaseContext service, and Tools/Options/Base/Registered
Databases), that is, a programmatic name which internally refers to some URL of
the .odb file.
Or, a data source name can the the URL directly (file:///some/path/to/file.odb).

Both should be freely interchangeable. A data transfer source is free to insert
any of both into the data access descriptor. Not sure which one is used by the
application controller, in theory, it is also free to use both.

I assume your question's background is you have a conflict with those two names
(one being a programmatic name, one being the URL)?

Also, what do you mean with "ranging from what time"?

en, "Ranging from what time" means "when not equals".:)

Because there is a judgement in OTableCopyHelper::insertTable:

			if ( _sSrcDataSourceName == _sDestDataSourceName )
				xSrcConnection = _xDestConnection;

.And this affect the default table name.
so I want to make it clear,then decided how to modified.

when both are not equal, this usually means they really refer to different
database documents.

However, there's one scenario where they're not equal, but refer to the same
document: If one is the programmatic name (such as "Bibliography"), and the
other is the file URL (such as "file:///.../biblio.odb") which is registered for
this programmatic name. This should happen relatively seldom, but is not impossible.

Created attachment 46271 [details]
modified

better, not yet there:

You now have a "new OCopyTable( ..., OCopyTableWizard::WIZARD_APPEND_DATA )
in two places, and you make this dependent on whether or not the
DefaultTableName is not empty.

The consequence:
OCopyTableWizard::m_eCreateStyle is not in sync with what you see in the UI.
That is, do the following:
- copy a table to the clipboard
- paste it, while it is selected
=> the wizard opens, and "Append" is pre-selected (which is fine)
- press "Next"
=> you get an error saying that the table name is already in use - which is
wrong, since you do not want to create this table, but append to it.

Please fix this scenario.

(side note: thanks for the all-in-one-file patch. Unfortunately, it still
requires multiple manual steps to apply it to a source tree. What I tried to
suggest above is something like
> cp -R dbaccess dbaccess.old
> (do changes in dbaccess)
> diff -ru dbaccess.bak dbaccess

Alternatively, if you have CVS access, a "cvs diff -u" in the root of dbaccess
would produce similar results.

With this, you would have one large patch file, which can be applied in one
single step.)

Created attachment 46685 [details]
new patch

Hi, Frank

The new patch file was made According the way you told me.Thank you :)!Is it 
that you need?

Best Regards.

I'll have a look at it, thank yoou very much. However, it might take 'til
beginning of next week until I find time, sorry.

ok.

okay, patch looks much better now :)

Two tiny little wishes, though:
- with setCreateStyleAction, you introduce a new method, which works fine, but
  is prone to future errors. That is, if somebody later finds this method
useful, and
  uses it for own purposes, the dialog/page might be left inconsistent: Consider
  somebody calling setCreateStyleAction( WIZARD_DEF ) - this would *not*
  be reflected in the dialog's CreateStyle
  In other words, I think setCreateStyleAction is missing a call to
  m_pParent->setCreateStyleAction.
  Speaking strictly, it is also missing a call to SetAppendDataRadio respectively
  RadioChangeHdl - those methods would also ensure the state of the remaining
  controls on the page fits the state of the check boxes.
- Please clean you patch from unnecessary whitespace changes (there are a
  few chunks which contain only changes in whitespaces), and unnecessary
  changes such as the new-but-commented-out line 140 in TableCopyHelper.hxx
Thanks.

Those two things addressed, I am willing to commit the patch.

ok.

Hi,Mr Frank:
:)Maybe I havn't understood your ideas.I think OCopyTable is only a page of 
OCopyTableWizard, so setCreateStyleAction is only used to set the check 
boxes's state in  OCopyTable,and SetAppendDataRadio is also only used to set 
the check boxs's state interrelated with append style.

so I had altered some of the following in OCopyTable:

void OCopyTable::setCreateStyleAction()//const 
OCopyTableWizard::Wizard_Create_Style& _eStyle
{
 // reselect the last action before
 switch(m_pParent->getCreateStyle())
 {
...
  case OCopyTableWizard::WIZARD_APPEND_DATA:
   m_aRB_AppendData.Check(sal_True);
   //m_pParent->EnableButton(OCopyTableWizard::WIZARD_NEXT,sal_False);
   SetAppendDataRadio();
...
},

in tablecopyhelper.cxx Changes are as follows :

    aWizard.fillTypeInfo();
    aWizard.loadData();
    if(GetIsSelectCopytable())
    {
     aWizard.ResetsName( GetDefaultTableName());
     aWizard.setCreateStyle(OCopyTableWizard::WIZARD_APPEND_DATA);
    }

    OCopyTable*   pPage1;
    pPage1 = new OCopyTable(&aWizard,COPY, bIsView);
    pPage1->setCreateStyleAction();
,

and I changed *SetAppendDataRadio* protected.  

well, my idea was the current code works fine, but it is easy to break it in
future. That is, we're talking about robustness of code here.

Consider the example I mentioned: if tomorrow, somebody does a
|setCreateStyleAction( WIZARD_DEF )| at the page, then he needs to also do a
|setCreateStyle| at the dialog. (Sorry, I mistyped this in my previous comment,
I wrongly wrote "m_pParent->setCreateStyleAction" instead of
"m_pParent->setCreateStyle".).

This coupling is prone to errors. Your new code would be more robust if the
implementation of |setCreateStyleAction| *atuomatically* would call
|setCreateStyle| at the dialog.

Okay, be it. If you would please provide me the complete actual patch, I'm
willing to check it in.

Created attachment 46989 [details]
new patch

Created attachment 46998 [details]
patch as committed to CWS dba24a

I committed the patch to CWS dba24a. Attached is the complete patch I committed,
you might want to compare it to your last patch - I did three changes:
- I removed some unnecessary whitespace changes
- in OCopyTable::SetAppendDataRadio, I re-inserted the call to
  "m_pParent->setCreateStyle". This is strictly necessary, else, pasting a
  table while there is no selection, and *then* selecting "Append data"
  in the wizard will lead to wrong results.
- I did changes to OCopyTable::setCreateStyleAction, so that it now also
  calls RadioChangeHdl where necessary. This ensures that if later on,
  RadioChangeHdl is changed in some way (for instance with en/disabling
  more controls), then this is automatically inherited by
  setCreateStyleAction.

patch committed to CWS dba24a. Thanks for submitting it!

targeting to 2.4, since the fix is part of a CWS aiming for this release

fs-> msc: please verify in CWS dba24a

verified in cws

This issue is closed automatically and wasn't rechecked in a current version of
OOo. This fixed issue should be integrated in OOo since more than half a year.
If you think this issue isn't fixed in a current version (OOo 3.1), please
reopen it and change the field 'Target Milestone' accordingly.

If you want to download a current version of OOo =>
http://download.openoffice.org/index.html
If you want to know more about the handling of fixed/verified issues =>
http://wiki.services.openoffice.org/wiki/Handle_fixed_verified_issues