olivierkes / manuskript

A open-source tool for writers
http://www.theologeek.ch/manuskript
GNU General Public License v3.0
1.74k stars 230 forks source link

Copy Bug in Outline and Editor #324

Open siliconserf opened 6 years ago

siliconserf commented 6 years ago

This was fun. I'm running 0.6.0 released, not any in progress version. The bug showed this way:

I had a sub-folder in the Outline tree that I copied into another folder. This didn't raise any flags, no idication of lack of ID, or ID duplication. I was able to edit each of the new documents in the folder copy. Then I saved and exited. The next time I tried to open the program I got the familiar Item ID problem in Revisions showing up in the Mate Terminal. Clearing that by editing the Revisions document cleared the ID complaint. Good, except another issue showed up - the project loaded, but the program complained in terminal window about a series of "MS" documents with duplicate IDs.

After trying closing the project and reloading, I decided the problem wasn't going to go away on its own. So, I used the Debug tab to access and edit one set of the duplicate ID "MS" documents, runumbering them from a number a bit higher than the highest ID I could find. I saved this and the next load of the project showed no problems with duplicate IDs.

This was a copy operation within the same program instance, not cross program instances. I haven't seen it happen when just copying several documents in the same folder, but I could have missed it. But, the point is there is a serious hole in the copy/ID handling code.

gedakc commented 6 years ago

I confirmed this issue with the following steps:

  1. Start manuskript and create a new project using the Novel template.
  2. Go to the Outline mode and create a new folder under Chapter 1.
  3. Select the New folder and create 3 new texts under the New folder.
  4. Right click on the New folder and select Copy.
  5. Select Chapter 2 and right click on it and select Paste.
  6. Close Manusrkipt.
  7. Start manuskript and open the project.
  8. Note messages similar to the following:
    $ bin/manuskript
    Debug: Web rendering engine used: QWebView
    Running manuskript version 0.6.0.
    Note: No translator found or loaded for locale en_CA.
    Loading: /home/gedakc/tmp/deleteme-2.msk
    Detected file format version: 1. Zip: True.
    WARNING ! There are some items with same IDs: ['18', '19', '20', '18', '19', '20']
    Project /home/gedakc/tmp/deleteme-2.msk loaded.
    Project deleteme-2.msk saved.

This problem also occurred with the latest develop branch.

Note that the same issue occurs when using Editor mode too.

src-r-r commented 6 years ago

@olivierkes I was looking at the design. I'd suggest, instead of using IDs, using UUIDs. This would (virtually) guarantee all items are unique. What are your thoughts? The only project overhead would be migrating the old ID code to UUIDs, which would just be a simple check on app launch. Because with the current design of assigning incremental numeric IDs I see 2 solutions for the copy problem:

  1. Keep track of the last ID created within the project. This would mean updating the ID with every operation, including copying, deleting, moving, etc.
  2. Check the ID with each operation, which results in n complexity.
gedakc commented 4 years ago

The duplicate ID problem arises if one copies and pastes a complete folder or scene because these new objects do not appear to be assigned a new ID. The problem does not arise if you copy only the written text from within a scene.

Personally I don't think that using UUID's alone will solve the problem because copying simple ID's or more complex UUID's and then pasting same should still result in duplicate values. Then again I've not dug deeply into the code so I could be wrong. This issue needs some love from someone with more knowledge of object oriented languages.

siliconserf commented 4 years ago

Yes, an object oriented approach would desirable. At least you'd get a constructor involved that would insure any new instance would have its own ID. I'd like to explore the subject. Is anyone using Eclipse to do development on this project?

On Sat, May 2, 2020 at 9:31 AM Curtis Gedak notifications@github.com wrote:

The duplicate ID problem arises if one copies and pastes a complete folder or scene because these new objects do not appear to be assigned a new ID. The problem does not arise if you copy only the text from within a scene.

Personally I don't think that using UUID's will solve the problem because copying simple ID's or more complex UUID's and then pasting same should still result in duplicate values. Then again I've not dug deeply into the code so I could be wrong. This issue needs some love from someone with more knowledge of object oriented languages.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-622979717, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FV2XR3KOUCAG2GKUWDRPRDELANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

gedakc commented 4 years ago

Mostly I use GNU emacs for development, but sometimes I use eclipse. For Manuskript I've found that the PyDev eclipse plugin works well for my purposes. Note that I had issues using the *ubuntu packages for eclipse so chose instead to install directly from the Eclipse web site packages.

siliconserf commented 4 years ago

I have Eclipse and the PyDev Eclipse plugin set up. How about a project file? It would save a lot of time.

On Sun, May 3, 2020 at 9:21 AM Curtis Gedak notifications@github.com wrote:

Mostly I use GNU emacs for development, but sometimes I use eclipse. For Manuskript I've found that the PyDev https://marketplace.eclipse.org/content/pydev-python-ide-eclipse eclipse plugin works well for my purposes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-623134809, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FSVVIIB6FU67DXOIFTRPWKX5ANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

gedakc commented 4 years ago

I suspect you are looking for more functionality than I have configured in eclipse. The steps I used to add the manuskript project to the eclipse workspace assume that you clone the manuskript repository into a directory to use as the eclipse workspace (mine is ~/workspace/).

  1. In Eclipse, select File -> New -> PyDev Project.
  2. In the dialog for the Project name enter the exact same name for the manuskript git repo that you have cloned into the workspace. For example: manuskript
  3. Click Finish button

This creates the following two files in the manuskript directory:

.project

<?xml version="1.0" encoding="UTF-8"?>
<projectDescription>
        <name>manuskript</name>
        <comment></comment>
        <projects>
        </projects>
        <buildSpec>
                <buildCommand>
                        <name>org.python.pydev.PyDevBuilder</name>
                        <arguments>
                        </arguments>
                </buildCommand>
        </buildSpec>
        <natures>
                <nature>org.python.pydev.pythonNature</nature>
        </natures>
</projectDescription>

.pydevproject

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<?eclipse-pydev version="1.0"?><pydev_project>
    <pydev_pathproperty name="org.python.pydev.PROJECT_SOURCE_PATH">
        <path>/${PROJECT_DIR_NAME}</path>
    </pydev_pathproperty>
    <pydev_property name="org.python.pydev.PYTHON_PROJECT_VERSION">python interpreter</pydev_property>
    <pydev_property name="org.python.pydev.PYTHON_PROJECT_INTERPRETER">Default</pydev_property>
</pydev_project>
siliconserf commented 4 years ago

Having an interesting time with this. Building manuskript gave an error reminding me to install pyQt5. Did that and now I get an error in version_0.py:

Line 13:from lxml import etree as ET

In the debugger: ModuleNotFoundError: No module named 'lxml'

I know that lxml showed up in a diagnostic over the file problem in the issue that kick off this adventure (thanks by the way for your indulging me with help on this.)

File "lxml.etree.pyx", line 2366, in lxml.etree._Attrib.getitem (src\lxml\lxml.etree.c:62258) KeyError: 'ID'

Since no file in the project answers to lxml I looked around and found lxml.py in a python directory. Is the IDE trying to tell me I have a configuration error?

On Sun, May 3, 2020 at 1:08 PM Curtis Gedak notifications@github.com wrote:

I suspect you are looking for more functionality than I have configured in eclipse. The steps I used to add the manuskript project to the eclipse workspace assume that you clone the manuskript repository into a directory to use as the eclipse workspace (mine is ~/workspace/).

  1. In Eclipse, select File -> New -> PyDev Project.
  2. In the dialog for the Project name enter the exact same name for the manuskript git repo that you have cloned into the workspace. For example: manuskript
  3. Click Finish button

This creates the following two files in the manuskript directory:

.project

<?xml version="1.0" encoding="UTF-8"?>

manuskript org.python.pydev.PyDevBuilder org.python.pydev.pythonNature

.pydevproject

<?xml version="1.0" encoding="UTF-8" standalone="no"?> <?eclipse-pydev version="1.0"?>

/${PROJECT_DIR_NAME}
<pydev_property name="org.python.pydev.PYTHON_PROJECT_VERSION">python interpreter</pydev_property>
<pydev_property name="org.python.pydev.PYTHON_PROJECT_INTERPRETER">Default</pydev_property>

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-623173090, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FWXUQAVGDIOP7HB7ADRPXFMJANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

siliconserf commented 4 years ago

Got it!!!! Found the problem was a missing install of lxml. Just ran manuskript without trouble. Thanks for your assistance and encouragement. Now I can experiment with the code and get a better understanding of the program (though if truth be told I really wish this were in C++ where I'm more comfortable.)

Regards,

Charlie H.

On Sun, May 3, 2020 at 9:17 PM Charlie H. siliconserf@gmail.com wrote:

Having an interesting time with this. Building manuskript gave an error reminding me to install pyQt5. Did that and now I get an error in version_0.py:

Line 13:from lxml import etree as ET

In the debugger: ModuleNotFoundError: No module named 'lxml'

I know that lxml showed up in a diagnostic over the file problem in the issue that kick off this adventure (thanks by the way for your indulging me with help on this.)

File "lxml.etree.pyx", line 2366, in lxml.etree._Attrib.getitem (src\lxml\lxml.etree.c:62258) KeyError: 'ID'

Since no file in the project answers to lxml I looked around and found lxml.py in a python directory. Is the IDE trying to tell me I have a configuration error?

On Sun, May 3, 2020 at 1:08 PM Curtis Gedak notifications@github.com wrote:

I suspect you are looking for more functionality than I have configured in eclipse. The steps I used to add the manuskript project to the eclipse workspace assume that you clone the manuskript repository into a directory to use as the eclipse workspace (mine is ~/workspace/).

  1. In Eclipse, select File -> New -> PyDev Project.
  2. In the dialog for the Project name enter the exact same name for the manuskript git repo that you have cloned into the workspace. For example: manuskript
  3. Click Finish button

This creates the following two files in the manuskript directory:

.project

<?xml version="1.0" encoding="UTF-8"?>

manuskript org.python.pydev.PyDevBuilder org.python.pydev.pythonNature

.pydevproject

<?xml version="1.0" encoding="UTF-8" standalone="no"?> <?eclipse-pydev version="1.0"?>

/${PROJECT_DIR_NAME}
<pydev_property name="org.python.pydev.PYTHON_PROJECT_VERSION">python interpreter</pydev_property>
<pydev_property name="org.python.pydev.PYTHON_PROJECT_INTERPRETER">Default</pydev_property>

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-623173090, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FWXUQAVGDIOP7HB7ADRPXFMJANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

-- Ceterum censeo, delenda est Trumpo

siliconserf commented 4 years ago

Having more fun digging into the code than I've had with software in some time. I tested my first impressions of the code by copying a manuskript project into the Sample directory. Lo and behold, the program listed "The-Book-of-Acts" as well as my dummy project and opened that project when clicked. Not a huge discovery, but it is nice to confirm some impressions as to what some of the code is doing, as well as knowing how to set up "template" projects. Thanks again for helping me get to this point.

Regards,

Charlie H.

On Tue, May 5, 2020 at 12:03 AM Charlie H. siliconserf@gmail.com wrote:

Got it!!!! Found the problem was a missing install of lxml. Just ran manuskript without trouble. Thanks for your assistance and encouragement. Now I can experiment with the code and get a better understanding of the program (though if truth be told I really wish this were in C++ where I'm more comfortable.)

Regards,

Charlie H.

On Sun, May 3, 2020 at 9:17 PM Charlie H. siliconserf@gmail.com wrote:

Having an interesting time with this. Building manuskript gave an error reminding me to install pyQt5. Did that and now I get an error in version_0.py:

Line 13:from lxml import etree as ET

In the debugger: ModuleNotFoundError: No module named 'lxml'

I know that lxml showed up in a diagnostic over the file problem in the issue that kick off this adventure (thanks by the way for your indulging me with help on this.)

File "lxml.etree.pyx", line 2366, in lxml.etree._Attrib.getitem (src\lxml\lxml.etree.c:62258) KeyError: 'ID'

Since no file in the project answers to lxml I looked around and found lxml.py in a python directory. Is the IDE trying to tell me I have a configuration error?

On Sun, May 3, 2020 at 1:08 PM Curtis Gedak notifications@github.com wrote:

I suspect you are looking for more functionality than I have configured in eclipse. The steps I used to add the manuskript project to the eclipse workspace assume that you clone the manuskript repository into a directory to use as the eclipse workspace (mine is ~/workspace/).

  1. In Eclipse, select File -> New -> PyDev Project.
  2. In the dialog for the Project name enter the exact same name for the manuskript git repo that you have cloned into the workspace. For example: manuskript
  3. Click Finish button

This creates the following two files in the manuskript directory:

.project

<?xml version="1.0" encoding="UTF-8"?>

manuskript org.python.pydev.PyDevBuilder org.python.pydev.pythonNature

.pydevproject

<?xml version="1.0" encoding="UTF-8" standalone="no"?> <?eclipse-pydev version="1.0"?>

/${PROJECT_DIR_NAME}
<pydev_property name="org.python.pydev.PYTHON_PROJECT_VERSION">python interpreter</pydev_property>
<pydev_property name="org.python.pydev.PYTHON_PROJECT_INTERPRETER">Default</pydev_property>

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-623173090, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FWXUQAVGDIOP7HB7ADRPXFMJANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

-- Ceterum censeo, delenda est Trumpo

-- Ceterum censeo, delenda est Trumpo

siliconserf commented 4 years ago

OK. One of the reasons I set up to play with the code was to track down the copy bug that revolved around failing to install a new ID number in the new copy. Only when I went to do so I found version 0.11.0 doesn't exhibit the problem. Change log entries don't seem to reflect this. Did the problem get fixed in PyQt?

Regards,

Charlie H.

On Tue, May 5, 2020 at 10:39 PM Charlie H. siliconserf@gmail.com wrote:

Having more fun digging into the code than I've had with software in some time. I tested my first impressions of the code by copying a manuskript project into the Sample directory. Lo and behold, the program listed "The-Book-of-Acts" as well as my dummy project and opened that project when clicked. Not a huge discovery, but it is nice to confirm some impressions as to what some of the code is doing, as well as knowing how to set up "template" projects. Thanks again for helping me get to this point.

Regards,

Charlie H.

On Tue, May 5, 2020 at 12:03 AM Charlie H. siliconserf@gmail.com wrote:

Got it!!!! Found the problem was a missing install of lxml. Just ran manuskript without trouble. Thanks for your assistance and encouragement. Now I can experiment with the code and get a better understanding of the program (though if truth be told I really wish this were in C++ where I'm more comfortable.)

Regards,

Charlie H.

On Sun, May 3, 2020 at 9:17 PM Charlie H. siliconserf@gmail.com wrote:

Having an interesting time with this. Building manuskript gave an error reminding me to install pyQt5. Did that and now I get an error in version_0.py:

Line 13:from lxml import etree as ET

In the debugger: ModuleNotFoundError: No module named 'lxml'

I know that lxml showed up in a diagnostic over the file problem in the issue that kick off this adventure (thanks by the way for your indulging me with help on this.)

File "lxml.etree.pyx", line 2366, in lxml.etree._Attrib.getitem (src\lxml\lxml.etree.c:62258) KeyError: 'ID'

Since no file in the project answers to lxml I looked around and found lxml.py in a python directory. Is the IDE trying to tell me I have a configuration error?

On Sun, May 3, 2020 at 1:08 PM Curtis Gedak notifications@github.com wrote:

I suspect you are looking for more functionality than I have configured in eclipse. The steps I used to add the manuskript project to the eclipse workspace assume that you clone the manuskript repository into a directory to use as the eclipse workspace (mine is ~/workspace/).

  1. In Eclipse, select File -> New -> PyDev Project.
  2. In the dialog for the Project name enter the exact same name for the manuskript git repo that you have cloned into the workspace. For example: manuskript
  3. Click Finish button

This creates the following two files in the manuskript directory:

.project

<?xml version="1.0" encoding="UTF-8"?>

manuskript org.python.pydev.PyDevBuilder org.python.pydev.pythonNature

.pydevproject

<?xml version="1.0" encoding="UTF-8" standalone="no"?> <?eclipse-pydev version="1.0"?>

/${PROJECT_DIR_NAME}
<pydev_property name="org.python.pydev.PYTHON_PROJECT_VERSION">python interpreter</pydev_property>
<pydev_property name="org.python.pydev.PYTHON_PROJECT_INTERPRETER">Default</pydev_property>

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-623173090, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FWXUQAVGDIOP7HB7ADRPXFMJANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

-- Ceterum censeo, delenda est Trumpo

-- Ceterum censeo, delenda est Trumpo

-- Ceterum censeo, delenda est Trumpo

gedakc commented 4 years ago

What OS and versions of Python and PyQt did you test with?

I reran the test steps from post 2 on Kubuntu 20.04 and the copy chapter/scene problem still exists. Details follow:

OS & Versions

Screenshot after Copy/Paste New Folder

Manuskript-issue324-kubuntu2004-outline

Console Log After Restarting Manuskript and Loading Project

user@kubuntu2004:~$ manuskript 
/usr/share/manuskript/manuskript/main.py:104: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if platform.system() is not 'Windows':
Debug: Web rendering engine used: QWebView
Running manuskript version 0.11.0.
Preferred translation: ['en-CA', 'en'] (based on available ui languages)
No translation found or loaded. (manuskript_en_CA.qm)
No translation found or loaded. (manuskript_en.qm)
Using the builtin translation.
/usr/share/manuskript/manuskript/importer/opmlImporter.py:124: SyntaxWarning: "is" with a literal. Did you mean "=="?
  return len(s) is 0
/usr/share/manuskript/manuskript/exporter/pandoc/abstractPlainText.py:78: SyntaxWarning: "is" with a literal. Did you mean "=="?
  if self.formats is "":
Icon theme "NumixMsk" not found.
Loading: /home/user/issue324-test.msk
Detected file format version: 1. Zip: True.
WARNING ! There are some items with same IDs: ['18', '19', '20', '18', '19', '20']
Project /home/user/issue324-test.msk loaded.
Project issue324-test.msk saved.
user@kubuntu2004:~$ 

Note that you can ignore the SyntaxWarning messages as these have been fixed in PR #762.

obw commented 4 years ago

I have programmed a little Proof-of-Concept in PHP, which cleans the dupe Id's.

Something like that, but in Python could be easily implemented and added too the Tool Menu or after closing the project, as long we could not solve this problem in the copy/paste code.

It was roughly two hour work for implementing this in PHP, but in Python I will need weeks:(

One thing I wanted too know is the speed for something like that:

This is only a prototype!
Make Backup before use!
(c) 2020 by Marcus Obwandner obwandner at googlemail.com
Licence: GNU General Public License https://www.gnu.org/licenses/gpl.html Version 3 or (at your option) any later version.
Found 3349 Files and 1358 files with a dupe ID

Do you have a Backup? Is the Projekt closed?
If for both yes! Press RETURN!

real    0m0,179s
user    0m0,059s
sys     0m0,082s

I have about 17MB text in this Project, outline, nothing else..

manuskript_id_clean_php.zip

No big magic done here, only one Parameter, path too the msk file.

First look if it's exists and proof if it's not a singlefile Manuskript-Project.

After that iterate over the Outline Dir where all the Texts exists and try to fetch all ID's out of them.

After that, when there are dupes wait for RETURN, else end.

Then iterate over the files with no or a dupe ID and give them a unique one. The first file with a ID which is used more then once is not changed.

It's a Prototype/Proof-of-Concept, so be aware that this thing can destroy your Project!

I have tested it with three different Projects and everything look perfect, but not in depth analyse is done!

siliconserf commented 4 years ago

I'm running Windows 10. Eclipse Java IDE 2 PyDev 7.5.0.202001101138

[image: image.png]

Python 3.8.2 (Opened News in the directory for Python and found the a text file that listed 3.8 as 3.8.2.

[image: image.png] I am certain that PyQt5 is on my computer, but having no luck finding it. Qt shows up in an MS cloud drive and others as Qt5. All of the development tools were installed two days ago. I don't think you can build and run Manuskript without those elements. If there is a way to interrogate Eclipse, or even my system for the information I'm game, even if we need to do a phone call and give you remote computer access.

From past experience I'm very familiar with the errors that get thrown up for the copy issue. If the source code hasn't been changed to address the copy issue, then something in Qt or Py has changed. I'm able to copy, save, exit, and restart Manuskript and re-load the test file without any problems.

One small detail: when I do a copy of a document, both the new and old md document files get their ID numbers embedded into the file names.

[image: image.png] Then

[image: image.png]

Copying two files, no matter to where gets the safe ID treatment. But copying a folder with documents still doesn't work properly. Files that get copied in the folders do not get new IDs and I get the error message. Interesting that just copying a file doesn't have a problem. Apparently the copy routine stops dealing with the ID issue when it encounters a folder.

Regards,

Charlie H. Everett, Wa., USA

On Fri, May 8, 2020 at 8:59 AM Curtis Gedak notifications@github.com wrote:

What OS and versions of Python and PyQt did you test with?

I reran the test steps from post 2 https://github.com/olivierkes/manuskript/issues/324#issuecomment-372386846 on Kubuntu 20.04 and the copy chapter/scene problem still exists. Details follow: OS & Versions

  • Kubuntu 20.04 Focal Fossa LTS
  • Python 3.8.2
  • PyQt 5.14.1
  • Qt 5.12.8

Screenshot after Copy/Paste New Folder

[image: Manuskript-issue324-kubuntu2004-outline] https://user-images.githubusercontent.com/10405019/81423959-69483480-9112-11ea-9889-014ae9b4f474.png Console Log After Restarting Manuskript and Loading Project

user@kubuntu2004:~$ manuskript /usr/share/manuskript/manuskript/main.py:104: SyntaxWarning: "is not" with a literal. Did you mean "!="? if platform.system() is not 'Windows': Debug: Web rendering engine used: QWebView Running manuskript version 0.11.0. Preferred translation: ['en-CA', 'en'] (based on available ui languages) No translation found or loaded. (manuskript_en_CA.qm) No translation found or loaded. (manuskript_en.qm) Using the builtin translation. /usr/share/manuskript/manuskript/importer/opmlImporter.py:124: SyntaxWarning: "is" with a literal. Did you mean "=="? return len(s) is 0 /usr/share/manuskript/manuskript/exporter/pandoc/abstractPlainText.py:78: SyntaxWarning: "is" with a literal. Did you mean "=="? if self.formats is "": Icon theme "NumixMsk" not found. Loading: /home/user/issue324-test.msk Detected file format version: 1. Zip: True. WARNING ! There are some items with same IDs: ['18', '19', '20', '18', '19', '20'] Project /home/user/issue324-test.msk loaded. Project issue324-test.msk saved. user@kubuntu2004:~$

Note that you can ignore the SyntaxWarning messages as these have been fixed in PR #762 https://github.com/olivierkes/manuskript/pull/762.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-625885493, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FR73ZFSFINT4P4KV7LRQQT7NANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

siliconserf commented 4 years ago

I'm game to try, but I've never worked with PHP. How do I launch this?

On Fri, May 8, 2020 at 12:23 PM obw notifications@github.com wrote:

I have programmed a little Proof-of-Concept in PHP, which cleans the dupe Id's.

Something like that, but in Python could be easily implemented and added too the Tool Menu or after closing the project, as long we could not solve this problem in the copy/paste code.

It was roughly two hour work for implementing this in PHP, but in Python I will need weeks:(

One thing I wanted too know is the speed for something like that:

This is only a prototype! Make Backup before use! (c) 2020 by Marcus Obwandner obwandner at googlemail.com Licence: GNU General Public License https://www.gnu.org/licenses/gpl.html Version 3 or (at your option) any later version. Found 3349 Files and 1358 files with a dupe ID

Do you have a Backup? Is the Projekt closed? If for both yes! Press RETURN!

real 0m0,179s user 0m0,059s sys 0m0,082s

I have about 17MB text in this Project, outline, nothing else..

manuskript_id_clean_php.zip https://github.com/olivierkes/manuskript/files/4601099/manuskript_id_clean_php.zip

No big magic done here, only one Parameter, path too the msk file.

First look if it's exists and proof if it's not a singlefile Manuskript-Project.

After that iterate over the Outline Dir where all the Texts exists and try to fetch all ID's out of them.

After that, when there are dupes wait for RETURN, else end.

Then iterate over the files with no or a dupe ID and give them a unique one. The first file with a ID which is used more then once is not changed.

It's a Prototype/Proof-of-Concept, so be aware that this thing can destroy your Project!

I have tested it with three different Projects and everything look perfect, but not in depth analyse is done!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-625976931, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FW3Q63DUDV7GDHQOXLRQRL2HANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

gedakc commented 4 years ago

@siliconserf it appears that the images you intended to include in the above post(s) are missing. Perhaps you emailed a response to github as opposed to writing a response in a web browser with this issue loaded?

The version details can be displayed when Manuskript is running using the Help -> About menu option.

Manuskript-issue324-kubuntu2004-about

EDIT:

I'm able to copy, save, exit, and restart Manuskript and re-load the test file without any problems.

I can also load the project after performing the copy, but the duplicate ID warning is displayed in the console log (not in manuskript, but in the terminal window / command prompt from which manuskript is invoked).

obw commented 4 years ago

@siliconserf

I'm game to try, but I've never worked with PHP. How do I launch this?

Well you need PHP on your machine first. If you don't have it by default, do not make the effort, it's more for developer.

If you want see how it's works just open it in a Text editor.

Line 66 to 80 iterates over the dirs and finds the dupes....

the loop in 103 make the cleanup...

function findNewId <- finds a unused id

function changeIdInFile <- change the datafile, also found a bug in this function, in line 134 is a return missing!

It as I said a proof of concept, with the hope that someone who is better in python implement it direct in Manuskript....

siliconserf commented 4 years ago

Yep. Forgot about that behavior. It tells me I have Python 3.4.4, PtQt 5.5.1, Qt 5.5.1

On Fri, May 8, 2020 at 1:34 PM Curtis Gedak notifications@github.com wrote:

@siliconserf https://github.com/siliconserf it appears that the images you intended to include in the above post(s) are missing. Perhaps you emailed a response to github as opposed to writing a response in a web browser with this issue loaded?

The version details can be displayed when Manuskript is running using the Help -> About menu option.

[image: Manuskript-issue324-kubuntu2004-about] https://user-images.githubusercontent.com/10405019/81446905-e71f3680-9138-11ea-81ab-626cbd52e3b4.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-626008450, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FXQZNSRHMMLTCAV6PDRQRUFLANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

siliconserf commented 4 years ago

And I'm down-rev on two of three. I will fix that shortly.

CH

On Fri, May 8, 2020 at 1:42 PM Charlie H. siliconserf@gmail.com wrote:

Yep. Forgot about that behavior. It tells me I have Python 3.4.4, PtQt 5.5.1, Qt 5.5.1

On Fri, May 8, 2020 at 1:34 PM Curtis Gedak notifications@github.com wrote:

@siliconserf https://github.com/siliconserf it appears that the images you intended to include in the above post(s) are missing. Perhaps you emailed a response to github as opposed to writing a response in a web browser with this issue loaded?

The version details can be displayed when Manuskript is running using the Help -> About menu option.

[image: Manuskript-issue324-kubuntu2004-about] https://user-images.githubusercontent.com/10405019/81446905-e71f3680-9138-11ea-81ab-626cbd52e3b4.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-626008450, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FXQZNSRHMMLTCAV6PDRQRUFLANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

-- Ceterum censeo, delenda est Trumpo

gedakc commented 4 years ago

And I'm down-rev on two of three. I will fix that shortly.

@siliconserf no need to change versions as those versions are still used in Kubuntu 16.04 LTS which is still a supported operating system.

The reason I asked about the versions is because we were trying to determine if a newer PyQt fixed the issue. From my testing it does not fix the problem. My thoughts are the the duplicate ID problem is an issue with the Manuskript source code and occurs with the copy and paste operation. That is ideally where it should be fixed.

I recommend ensuring that you can reproduce the bug before trying to fix the code.

siliconserf commented 4 years ago

Now that I know a folder copy is needed I think I can track it down.

On Fri, May 8, 2020 at 2:33 PM Curtis Gedak notifications@github.com wrote:

And I'm down-rev on two of three. I will fix that shortly.

@siliconserf https://github.com/siliconserf no need to change versions as those versions are still used in Kubuntu 16.04 LTS which is still a supported operating system.t

The reason I asked about the versions is because we were trying to determine if a newer PyQt fixed the issue. From testing it does not fix the problem. My thoughts are the the duplicate ID problem is an issue with the Manuskript source code and occurs with the copy and paste operation. That is ideally where it should be fixed.

I recommend ensuring that you can reproduce the bug before trying to fix the code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-626030628, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FRI6PRGTYFKBIBVGHDRQR3CNANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

gedakc commented 4 years ago

Excellent. Happy bug hunting!

siliconserf commented 4 years ago

Interesting results. I find that sometimes I can copy a folder with files without an ID duplication occurring. And there is definitely a lag between copying and when ID numbers are rationalized. The copied file folders and documents initially have no IDs, then magically the program catches up and no problem. This bug is a bit more complex than I had imagined. At least more subtle. This is outside the IDE. I'm wondering if the ID code is thread safe. Or any of the code for that matter.

siliconserf commented 4 years ago

Here is my "stalking horse" project copied out of the Edit function using the tool Snipy.

image

The section Scenes was copied and pasted into Notes. No duplicated IDs. No problem reloading after shutting down. Previously I'd copied Chapter Two and then edited the document and folder names. No problem their either.

gedakc commented 4 years ago

I'm wondering if the ID code is thread safe. Or any of the code for that matter.

Previously PR #706 fixed a bug with multi-threading so at least that part of the code was previously not thread safe. It is possible that a similar issue resides in the ID code.

Note that I re-opened this issue (I'm guessing it was accidentally closed).

siliconserf commented 4 years ago

Looks as though I need to study mutexes in Python code. And spend more time just browsing through the code to understand it's operational structure.

On Sun, May 10, 2020 at 8:46 AM Curtis Gedak notifications@github.com wrote:

I'm wondering if the ID code is thread safe. Or any of the code for that matter.

Previously issue #706 https://github.com/olivierkes/manuskript/pull/706 fixed a bug with multi-threading so at least that part of the code was previously not thread safe. It is possible that a similar issue resides in the ID code.

Note that I re-opened this issue (I'm guessing it was accidentally closed).

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-626347513, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FSMI2ZF5QO3KXIA743RQ3D4VANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

gedakc commented 4 years ago

Re-opening this issue as it appears to have been accidentally closed, and the problem with the creation of duplicate ID's when copying and pasting still exists.

siliconserf commented 4 years ago

I set up a small project to use in testing the copy bug. What I am seeing is when a folder with three text children is copied, the ID's get stripped when pasting (which is correct). I tracked the process to the point where the code decides whether to install a unique ID. But the test for no ID being present always fails! See abstractItem line 139:

    if not child.ID():
        child.getUniqueID()

I think the code sees that the property is defined instead of testing to see if an ID has been assigned. To verify this I tried to change the test to one that explicitly checks the value of the ID, but my command of Python code is woefully short. Can you help with the correct code to test the ID? Should there be a two stage test, first for the property existing, and then for the value?

gedakc commented 4 years ago

Good work @siliconserf. From a quick look it appears that the copy only strips the folder ID, but the three child text ID's remain set. Hence when these are pasted they are not given a new ID.

EDIT: The problem might occur earlier with the copy operation not clearing all the children (and lower) ID's.

siliconserf commented 4 years ago

I'm not seeing quite that as I debug. When the children of the folder I copy are added to the database no ID is set. The string is empty. Doing a save immediately after the copy gives me md text files with no ID field.

gedakc commented 4 years ago

I'm using the following print statements to track what is occurring:

$ git diff
diff --git a/manuskript/models/abstractItem.py b/manuskript/models/abstractItem.py
index f3fa049..e5c0861 100644
--- a/manuskript/models/abstractItem.py
+++ b/manuskript/models/abstractItem.py
@@ -137,6 +137,9 @@ class abstractItem():
         child.setModel(self._model)
         if not child.ID():
             child.getUniqueID()
+            print("DEBUG: New Unique Child ID: " + child.ID())
+        else:
+            print("DEBUG: Old Child ID: " + child.ID())

     def removeChild(self, row):
         """

Using the test steps I get the following output:

Creating the project with step 1:

Debug: Web rendering engine used: QWebView
Running manuskript version 0.11.0.
Preferred translation: builtin (based on user setting)
Using the builtin translation.
<snip>
DEBUG: New Unique Child ID: 1
DEBUG: New Unique Child ID: 2
DEBUG: New Unique Child ID: 3
<snip>
DEBUG: New Unique Child ID: 118
DEBUG: New Unique Child ID: 119
DEBUG: New Unique Child ID: 120
Project deleteme-4.msk saved.

Perform step 2 to create the folder:

DEBUG: New Unique Child ID: 121

Perform step 3 to create the three texts:

DEBUG: New Unique Child ID: 122
DEBUG: New Unique Child ID: 123
DEBUG: New Unique Child ID: 124
Project deleteme-4.msk saved.

Perform step 4 to copy and step 5 to paste the folder:

DEBUG: Old Child ID: 122
DEBUG: Old Child ID: 123
DEBUG: Old Child ID: 124
DEBUG: New Unique Child ID: 125
Project deleteme-4.msk saved.
Project deleteme-4.msk saved.

Note above that only one item (the folder) is given a new ID. The three texts remain with the old IDs.

EDIT:

Can you help with the correct code to test the ID? Should there be a two stage test, first for the property existing, and then for the value?

From the above console log, the three texts do have an ID and each text has a unique ID, so I don't think we need a two stage test here.

I do wonder why the three texts have blank IDs in the .md files when you perform the save. I wonder if there is more than one problem we are looking at?

siliconserf commented 4 years ago

Yes, there are at least two bugs. But I think we can squish all variations of them by re-thinking how the code handles cut/copy/move operations.

In both your and my test cases, when the code drops clipboard data into place it only assigns a new ID to the top folder. No descent into the children. Your test somehow doesn't strip the child IDs, and in my test case the tree is recursively stripped.

I think the bug is in when the code chooses to strip the IDs. Currently it happens as a block separately from the actual insertion. That opened up opportunities for it not being reliably invoked it shouldn't be called at all.

Check me on this: A Cut operation removes the selected document(s) and folders from the working tree and holds them in the clipboard buffer. A Copy operation leaves copies in place. In between either a copy or cut operation and a paste, you could, in theory, create, move, or delete any number of files or folders without altering the clipboard.

So, the only reliable way to know if pasting from the clipboard is actually part of a copy operation is by having Paste check each file and folder to see if its ID exists in the Root tree as it performs each insertion item by item into the Root tree. Since a Cut has already removed the possibility of an orphan ID, this approach eliminates any possible null or duplicated ID. It should correct all variations of the copy/paste bug we've seen.

ID-Horse.zip

gedakc commented 4 years ago

Minor mention: I reopened this issue again. Are you perhaps clicking on Close and Comment instead of Comment? Or maybe something else occuring?

Now onto the duplicate ID copy problem at hand.

With your help I think we are zeroing in on the issue. I think that the paste action actually inserts/creates the three child text objects in the project before the three text ID's are stripped.

To debug I used the following print statements:

$ git diff
diff --git a/manuskript/models/abstractItem.py b/manuskript/models/abstractItem.py
index f3fa049..c4def1e 100644
--- a/manuskript/models/abstractItem.py
+++ b/manuskript/models/abstractItem.py
@@ -137,6 +137,9 @@ class abstractItem():
         child.setModel(self._model)
         if not child.ID():
             child.getUniqueID()
+            print("DEBUG: New Unique Child ID: " + child.ID())
+        else:
+            print("DEBUG: Old Child ID: " + child.ID())

     def removeChild(self, row):
         """
@@ -182,6 +185,7 @@ class abstractItem():
         Returns a copy of item, with no parent, and no ID.
         """
         item = self.__class__(xml=self.toXML())
+        print("DEBUG: copy self.enum.ID: " + self.enum.ID)
         item.setData(self.enum.ID, None)
         return item

diff --git a/manuskript/models/abstractModel.py b/manuskript/models/abstractModel.py
index a536084..8f65235 100644
--- a/manuskript/models/abstractModel.py
+++ b/manuskript/models/abstractModel.py
@@ -422,6 +422,7 @@ class abstractModel(QAbstractItemModel):
                 if item.ID() in IDs:
                     # Recursively remove ID. So will get a new one when inserted.
                     def stripID(item):
+                        print("DEBUG CopyAction strip ID: " + item.ID())
                         item.setData(Outline.ID, None)
                         for c in item.children():
                             stripID(c)

Using the test steps I get the following output:

  1. Create novel project:

    $ bin/manuskript 
    Debug: Web rendering engine used: QWebView
    Running manuskript version 0.11.0.
    Preferred translation: builtin (based on user setting)
    Using the builtin translation.
    <snip>
    DEBUG: New Unique Child ID: 1
    DEBUG: New Unique Child ID: 2
    DEBUG: New Unique Child ID: 3
    <snip>
    DEBUG: New Unique Child ID: 118
    DEBUG: New Unique Child ID: 119
    DEBUG: New Unique Child ID: 120
    Project deleteme-4.msk saved.
  2. Create folder:

    DEBUG: New Unique Child ID: 121
    Project deleteme-4.msk saved.
  3. Create three texts:

    DEBUG: New Unique Child ID: 122
    DEBUG: New Unique Child ID: 123
    DEBUG: New Unique Child ID: 124
    Project deleteme-4.msk saved.
  4. Copy folder:

    no output

    Aside: I had thought that the DEBUG: copy self.enum.ID statement would appear in the console log. Then again I'm not expert on the code so I don't know for sure what this is doing.

  5. Paste folder:

    DEBUG: Old Child ID: 122
    DEBUG: Old Child ID: 123
    DEBUG: Old Child ID: 124
    DEBUG CopyAction strip ID: 121
    DEBUG CopyAction strip ID: 122
    DEBUG CopyAction strip ID: 123
    DEBUG CopyAction strip ID: 124
    DEBUG: New Unique Child ID: 125
    Project deleteme-4.msk saved.

    Note that the logging of DEBUG: Old Child ID's from insertChild() occurs before the DEGUG: CopyAction strip ID's from dropMimeData().

siliconserf commented 4 years ago

You guessed it, I hit the close button thinking it was just closing my view of the issue rather than trying to tie off the topic.

Yes, I think you're right about the items getting inserted first and then IDs getting stripped with the expectation the code would then drill down and apply IDs. Seems there are two bugs, failing to strip, and failing to apply new recursively.

This whole experience has me wondering why the items have IDs in the first place. They would seem to lack a compelling reason for existing.

On Wed, May 13, 2020 at 10:06 AM Curtis Gedak notifications@github.com wrote:

Minor mention: I reopened this issue again. Are you perhaps clicking on Close and Comment instead of Comment? Or maybe something else occuring?

Now onto the duplicate ID copy problem at hand.

With your help I think we are zeroing in on the issue. I think that the paste action actually inserts/creates the three child text objects in the project before the three text ID's are stripped.

To debug I used the following print statements:

$ git diff diff --git a/manuskript/models/abstractItem.py b/manuskript/models/abstractItem.py index f3fa049..c4def1e 100644 --- a/manuskript/models/abstractItem.py +++ b/manuskript/models/abstractItem.py @@ -137,6 +137,9 @@ class abstractItem(): child.setModel(self._model) if not child.ID(): child.getUniqueID()

  • print("DEBUG: New Unique Child ID: " + child.ID())
  • else:
  • print("DEBUG: Old Child ID: " + child.ID())

    def removeChild(self, row): """ @@ -182,6 +185,7 @@ class abstractItem(): Returns a copy of item, with no parent, and no ID. """ item = self.class(xml=self.toXML())

  • print("DEBUG: copy self.enum.ID: " + self.enum.ID) item.setData(self.enum.ID, None) return item

diff --git a/manuskript/models/abstractModel.py b/manuskript/models/abstractModel.py index a536084..8f65235 100644 --- a/manuskript/models/abstractModel.py +++ b/manuskript/models/abstractModel.py @@ -422,6 +422,7 @@ class abstractModel(QAbstractItemModel): if item.ID() in IDs:

Recursively remove ID. So will get a new one when inserted.

                 def stripID(item):
  • print("DEBUG CopyAction strip ID: " + item.ID()) item.setData(Outline.ID, None) for c in item.children(): stripID(c)

Using the test steps https://github.com/olivierkes/manuskript/issues/324#issuecomment-372386846 I get the following output:

1.

Create novel project:

$ bin/manuskript Debug: Web rendering engine used: QWebView Running manuskript version 0.11.0. Preferred translation: builtin (based on user setting) Using the builtin translation.

DEBUG: New Unique Child ID: 1 DEBUG: New Unique Child ID: 2 DEBUG: New Unique Child ID: 3 DEBUG: New Unique Child ID: 118 DEBUG: New Unique Child ID: 119 DEBUG: New Unique Child ID: 120 Project deleteme-4.msk saved. 2. Create folder: DEBUG: New Unique Child ID: 121 Project deleteme-4.msk saved. 3. Create three texts: DEBUG: New Unique Child ID: 122 DEBUG: New Unique Child ID: 123 DEBUG: New Unique Child ID: 124 Project deleteme-4.msk saved. 4. Copy folder: *no output* Aside: I had thought that the *DEBUG: copy self.enum.ID * statement would appear in the console log. Then again I'm not expert on the code so I don't know for sure what this is doing. 5. Paste folder: DEBUG: Old Child ID: 122 DEBUG: Old Child ID: 123 DEBUG: Old Child ID: 124 DEBUG CopyAction strip ID: 121 DEBUG CopyAction strip ID: 122 DEBUG CopyAction strip ID: 123 DEBUG CopyAction strip ID: 124 DEBUG: New Unique Child ID: 125 Project deleteme-4.msk saved. Note that the logging of *DEBUG: Old Child ID's* from insertChild() occurs *before* the *DEGUG: CopyAction strip ID's* from dropMimeData(). — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub , or unsubscribe .

-- Ceterum censeo, delenda est Trumpo

gedakc commented 4 years ago

This whole experience has me wondering why the items have IDs in the first place. They would seem to lack a compelling reason for existing.

I believe every item has an ID so that it can be associated with other IDs. For example a Text (scene) can be associated with several Characters or Plots. This can be useful for features like the Storyline.

I think the problem stems from an issue with the Copied ID's not all being stripped before the Insert operation. In my mind it is a problem with the order of operations. My background is in procedural languages that follow the order of the code. However in this case we are dealing with event driven code and objects, so the order is never guaranteed. At least those are my thoughts.

I think if we can somehow enforce that the stripping of ID's occurs before the objects are inserted then we will have a fix for the problem.

siliconserf commented 4 years ago

Yes, files with Character or Plot links point to the respective entries, so either we play the ID game or use hyperlinks (which I think has a lot of advantages).

But more to the point we're fixing at the moment - I agree about forcing the strip before insertion. But I think it must be done item by item with a Qmutex used to prevent any other event driven code from changing the data base while you're busy stuffing each item place. Maybe the loop that currently strips the IDs in the list should Qmutext calls into single item pastes? I gotta go back into the code.

On Wed, May 13, 2020 at 10:41 AM Curtis Gedak notifications@github.com wrote:

This whole experience has me wondering why the items have IDs in the first place. They would seem to lack a compelling reason for existing.

I believe every item has an ID so that it can be associated with other IDs. For example a Text (scene) can be associated with several Characters or Plots. This can be useful for features like the Storyline https://www.theologeek.ch/manuskript/2016/02/28/story-line/.

I think the problem stems from an issue with the Copied ID's not all being stripped before the Insert operation. In my mind it is a problem with the order of operations. My background is in procedural languages that follow the order of the code. However in this case we are dealing with event driven code and objects, so the order is never guaranteed. At least those are my thoughts.

I think if we can somehow enforce that the stripping of ID's occurs before the objects are inserted then we will have a fix for the problem.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-628142494, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FURZVNI2IW4U5IT5BLRRLLTHANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

siliconserf commented 4 years ago

While studying the code to understand where and how to force null IDs to get a unique value I stumbled across a problem with "Outline" in the XML procedure:

def setFromXMLProcessMore(self, root):

    # If loading from an old file format, convert to md and
    # remove html markup
    if self.type() in ["txt", "t2t"]:
        self.setData(Outline.type, "md")  <======= Eclipse complains "Outline" is an undefined variable. Shouldn't this be a test of "outlineItem.type"? Similarly below.

    elif self.type() == "html":
        self.setData(Outline.type, "md")
        self.setData(Outline.text, HTML2PlainText(self.data(Outline.text)))
        self.setData(Outline.notes, HTML2PlainText(self.data(Outline.notes)))

    # Revisions
    for child in root:
        if child.tag == "revision":
            self.appendRevision(child.attrib["timestamp"], child.attrib["text"])

From the comments in the file this function seems to be here to help with importing project data from much earlier versions. If I'm correct in my understanding, then this function hasn't been called in quite a while, if ever. In any event I don't think I have a project from an old enough version to even begin to try to debug this.

gedakc commented 4 years ago

The Open and plain-text file format feature article is dated March 31, 2016. Looking through the official releases it seems to have been implemented in version 0.3.0.

I started to look at Manuskript starting with the 0.3.0 release, so I don't have any of the old file format projects either.

I haven't looked at this section of code, and as you indicated it is likely seldom if ever used. If you need to fix it to continue debugging then feel free to do so. Eventually we'll want to remove this old code, but for now I think it is useful to keep as an example for when the next file format change is implemented.

siliconserf commented 4 years ago

OK, I think I have a fix that stops copying a tree from leaving item IDs nulled. Interested in testing my fix to abstractModel? I'm still pretty new to Python so my fix might not be all that elegant.

abstractModel.zip

BTW - While debugging this problem I noticed that if a project is loaded with items that have no ID, the errant items get one assigned, but not reliably. That's another issue to track down. BUT, a big, related problem I stumbled upon is that the Object editor will allow you to copy and paste a tree of folders and files back into the same tree. This could happen accidentally. I think the copy in the clipboard still has links back into the Object tree, and so when processed you get a recursive explosion. That's peculation at this point. I haven't tried to test the theory.

gedakc commented 4 years ago

Good work! From a quick test your proposed code changes do seem to address the copy folder yields duplicate ID problem. :-)

I'll do some more testing with debug statements to better understand the changes.

While performing a git diff I noticed lots of unnecessary whitespace in the abstractModel.py file. This extraneous whitespace should be cleaned up so that only the key changes are highlighted which helps with later debugging.

Are you planning to submit a Pull Request?

gedakc commented 4 years ago

In further testing I discovered an issue. When I copy a folder with texts that are nested within a folder with texts I encounter errors.

The folder structure I'm trying to copy is as shown in the following screenshot:

Manuskript-Copy-Bug-Testing

The error messages I receive are:

Traceback (most recent call last):
  File "/home/gedakc/workspace/manuskript.gedakc/bin/../manuskript/ui/views/outlineBasics.py", line 302, in paste
    self.model().dropMimeData(mimeData, Qt.CopyAction, -1, 0, index)
  File "/home/gedakc/workspace/manuskript.gedakc/bin/../manuskript/models/abstractModel.py", line 449, in dropMimeData
    backfillId(item.children())
  File "/home/gedakc/workspace/manuskript.gedakc/bin/../manuskript/models/abstractModel.py", line 445, in backfillId
    backfillId(c)        # Process its children for null IDs
  File "/home/gedakc/workspace/manuskript.gedakc/bin/../manuskript/models/abstractModel.py", line 443, in backfillId
    for c in item:           # Go through all items at this level
TypeError: 'outlineItem' object is not iterable

Note that the line numbers won't likely match your code exactly because I have added debug print statements in my code.

siliconserf commented 4 years ago

The error message sounds like the interpreter is objecting to my not testing first to see if there are any children before running the for loop. I had thought the interpreter would have dealt with that question by aborting the loop. I'll see if adding a test for children before the for loop fixes the problem.

Whitespace: I'll suck out a lot of that.

I've never done a pull request so when we agree this is ready for adding to the development tree you can walk me through that.

CH

On Thu, May 21, 2020 at 9:50 AM Curtis Gedak notifications@github.com wrote:

In further testing I discovered an issue. When I copy a folder with texts that are nested within a folder with texts I encounter errors.

The folder structure I'm trying to copy is as shown in the following screenshot:

[image: Manuskript-Copy-Bug-Testing] https://user-images.githubusercontent.com/10405019/82583737-af5cb980-9b50-11ea-9700-c5b29688c4d4.png

The error messages I receive are:

Traceback (most recent call last): File "/home/gedakc/workspace/manuskript.gedakc/bin/../manuskript/ui/views/outlineBasics.py", line 302, in paste self.model().dropMimeData(mimeData, Qt.CopyAction, -1, 0, index) File "/home/gedakc/workspace/manuskript.gedakc/bin/../manuskript/models/abstractModel.py", line 449, in dropMimeData backfillId(item.children()) File "/home/gedakc/workspace/manuskript.gedakc/bin/../manuskript/models/abstractModel.py", line 445, in backfillId backfillId(c) # Process its children for null IDs File "/home/gedakc/workspace/manuskript.gedakc/bin/../manuskript/models/abstractModel.py", line 443, in backfillId for c in item: # Go through all items at this level TypeError: 'outlineItem' object is not iterable

Note that the line numbers won't likely match your code exactly because I have added debug print statements in my code.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-632219606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FQLW4P5IU7MMOZTIIDRSVLWLANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

siliconserf commented 4 years ago

Please try the two files attached. Main.py fixes a complaint from Eclipse's debugger about "not equals" not correct for the usage in a platform test. I changed that accordingly and haven't seen any repercussions. Also, the latest version of my fix seems to deal fine with copying folders with trees of files. I'd like that tested.

The fix for duplicated/non-ID's files was to modify the loop that stripped existing IDs from items that were being copied. The code assumed the insertion routine would take care of the IDs, but it didn't. Since the old ID values were going anyway I just plunked in new unique ID values.

Copying a tree within itself gets copied after the tree. The code is smart about that.

Bug 324 Fix try B.zip

gedakc commented 4 years ago

Thanks @siliconserf for continuing to work on this problem. The fix you included in main.py is a part of PR #762, so no need to include it in your patches.

One note is that history of changes and authors is automatically tracked in git so there is no need to place initialed and dated entries in the comments.

For an example of how to use git from another project I work on, see Developing GParted using Git. The important stuff to note is how to set up git and commit code with appropriate comments. The stuff on compiling and testing is specific to the GParted project.

Note that I haven't tested your code yet, but wanted you to have some time to investigate how to use git. See also GitHub.com: Development workflow with Git: Fork, Branching, Commits, and Pull Request

Also thanks @obw - I assume your thumbs up means you tested the code?

siliconserf commented 4 years ago

Glad to have independent testing of the fix. I'm still very much a novice with Python coding.

While testing I think I found a bug hiding in the code that selects multiple things to copy or move. Somehow an incorrect keyboard input combination injected something that killed a test manuskript project so the program crashes on loading the project. That needs more exploring to see just how it dies. I'll post about it when I can reliably duplicate the problem.

Git is installed so I'll explore working with it. I had tagged where I changed the code so you could find it easily. Having Git track that sort of thing should keep the code from getting cluttered with too much in the way of comments so I can go along with that way of documenting changes.

On Wed, May 27, 2020 at 10:07 AM Curtis Gedak notifications@github.com wrote:

Thanks @siliconserf https://github.com/siliconserf for continuing to work on this problem. The fix you included in main.py is a part of PR #762 https://github.com/olivierkes/manuskript/pull/762, so no need to include it in your patches.

One note is that history of changes and authors is automatically tracked in git so there is no need to place initialed and dated entries in the comments.

For an example of how to use git from another project I work on, see Developing GParted using Git https://gparted.org/git.php. The important stuff to note is how to set up git and commit code with appropriate comments. The stuff on testing is specific to the GParted project.

Note that I haven't tested this code yet, but wanted you to have some time to investigate how to use git. See also GitHub.com: Development workflow with Git: Fork, Branching, Commits, and Pull Request https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/Development-workflow-with-Git:-Fork,-Branching,-Commits,-and-Pull-Request

Also thanks @obw https://github.com/obw - I assume your thumbs up means you tested the code?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-634807381, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FVS4B4R6CED46GUILLRTVCDVANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo

siliconserf commented 4 years ago

OK. Here is what I've done. I installed Git along with Desktop GUI front end. Then set up a local clone of the Development branch in a local drive repository. Next, I installed the edited copy of abstractModel.py in that repository, then created a branch and pushed that to GitHub as Issue-324-NoID-Fix. If I understood how Git works you should be able to see that proposed change and we can proceed from there. Hope I haven't blown up anything.

gedakc commented 4 years ago

When I looked at Manuskript Pull Requests I did not observe a PR from you.

If you've pushed a new branch to your personal GitHub repository then perhaps you missed a step to login to GitHub and create a pull request?

siliconserf commented 4 years ago

OK, I stumbled across the correct incantation using GitHub Desktop and got a pull request created.

On Thu, Jun 4, 2020 at 8:45 AM Curtis Gedak notifications@github.com wrote:

When I looked at Manuskript Pull Requests https://github.com/olivierkes/manuskript/pulls I did not observe a PR from you.

If you've pushed a new branch to your personal GitHub repository then perhaps you missed a step to login to GitHub and create a pull request https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/olivierkes/manuskript/issues/324#issuecomment-638937848, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVW6FSDRWNLPUXIW44BU7LRU66RPANCNFSM4EUPK3XQ .

-- Ceterum censeo, delenda est Trumpo