GROUP_PLAIN_AND_RTF_RESOURCES_R0

[deyan]

GROUP_PLAIN_AND_RTF_RESOURCES_R0

Ticket	Summary	Effort	Status
#831	TEXT_RESOURCE_HTML_R0	1	closed
#835	TEXT_RESOURCE_RTF_R0	1	closed
#839	TEXT_RESOURCE_PLAIN_R0	0.5	closed

[dido]

Is it possible to create reload functionality and origin property for this resources. Think that it's not implemented yet for any resource, but we have to attack this issue some day.
Also the rtf file should preserve formatting and styles - expect that in next revision of this group it will be clear, what part of this will be possible to implement.
Analysis review 4p (20m)

[meddle]

So I told you what to fix but I'll write it here again:

• You use the javax.swing.text library, you can explain more in details it's use and give a link to it's JavaDoc.
• When you add new classes you have to write where they will be added.
• This ImportUtil should be a utility for all kinds of frames, the part about HotText must be in the HotText related modules, maybe in org.sophie2.main.func.text.
• Create a ContentProvider for the HotTextFrameContent like with all the other contents, somebody have to do it, and you change the "insert frame" logic, so...
• About the code :) I didn't forget the "TEXT_RESOURCE_HTML_R0" design comments :), so:
  • The assertEquals(buf.toString(),this.text); method has special arguments the first one is the EXPECTED one the second the ACTUAL, your case is not that one.
  • The indent your code, it's hardly readable that way.
  • Write your JavaDoc to be readable, put one new line between it's overview and parameters, indent it, write it carefully to be helpful, the good JavaDoc can be used as design documentation as well.
  • I think you should test your LogicR3 handler as well with SystemTest.
  • The current test of yours is not good, may be it will be no automatic, that method - AppLogic.get().userCreateNewBook(mw.appView().get()); will make you use your BookPropertiesDialog, that's not good, in the SystemTest class there is a method for creating new book without it, you can use it in the SystemTest that you will write, and it's code when fixing this test that will test only the HtmlUtility logic and not use this many modules.
  • Are you diana or Diana, the author identificator is important thing.
  • Blocks like bodies of "if"s even if they are one row are surrounded with curly brackets.
  • Think of better names for your variables, one letter names are bad practice...
  • Flying JavaDoc is bad thing
  • I know that you are familiar with that page GoodCodeExamples, I don't know why don't you want to write your code using it, the other people do it and their reviews pass easily.
  • Why you write this twice : htmlResource.text().set(text);

Ummm so I'll fail it for now, but if you fix this thing it will pass, and please sort the changesets...

2p (40m)

[meddle]

Design related notes:

• The names of your modules are nit good when we have in mind our conventions. Rename them:
  • org.sophie2.extra.func.plainText -> org.sophie2.extra.func.text.plain or org.sophie2.extra.func.plain
  • org.sophie2.extra.func.rtfText -> org.sophie2.extra.func.text.rtf or org.sophie2.extra.func.rtf
• The HotTextImportUtil is in the org.sophie2.main.func.text module, but in your design you say that it's in the html module. Fix that thing!

Code related notes:

• I took a look at your new content provider. Your code is not merged with the trunk a least form one week, which can mean that is not mergeable trivially, so I recommend you to synchronize your branch... But that is for implementation phase ofcaurse.
• That is the worse method for creating a ResourceRef -> ResourceRef.getReferenceByIdAndLocation
• The @author in the JavaDoc is bellow everything else in it except the parameters of the generic classes.
• "if" and "for" expressions, even with only one row body must be surrounded with curly brackets.
• Bad spacing... The code must be easy to read.
• Don't use e.printStackTrace();, use the logger with second argument the Throwable. Example logger.debug("my message", e);
• You know that JavaDoc with empty @param expressions is unacceptable, even for getters and setters like:

   /**
    * Setter for the value. 
    *  
    * @param value 
    */ 
  

You can also see that between the first text line and the start point of the JavaDoc, there is one empty line.

I believe you will fix the things that I wrote you bellow for the implementation review, so I'll pass the design. I think now it's clean what you want to do. And again... you are the design owner, write your identificator in the fields.

3.5p (1h)

[diana]

About:That is the worse method for creating a ResourceRef -> ResourceRef.getReferenceByIdAndLocation - tell me a better one...

[diana]

About:"if" and "for" expressions, even with only one row body must be surrounded with curly brackets - I have searched for one of those for nearly half an hour...and I don't see one...if you tell me where to find it , I'll fix it.

[meddle]

There are such things. :)

[meddle]

Problems:

• Fix your design to be with the new package names.
• A better ref is the getRelativeRef, and is very important for the ContentProviders, see the other ones. If you don't know how to use something ask, before you use it.
• Describe your provider in the design, It's important to be mentioned in it.
• Rename the HotTextImportUtil and it's descendants to HotTextImportManager and etc...
• Use the proper change managers:

  // WRONG:
  
                 resource = ResourceSpace.createResource(resFile.getName(),
                              HotTextBookResource.class, book);
                  HotText text = HotTextFactory.createHotText(DefaultChangeManager.get());
  

  // RIGHT:
  
                  resource = ResourceSpace.createResource(resFile.getName(),
                              HotTextBookResource.class, book);
                  HotText text = HotTextFactory.createHotText(resource.getChangeManager());
  

• I already fixed one of your try -> catch(Exception code), but we will not torelate such things... Yous conditions for such stuff; Example:

                  try {
  	            Reader r = new FileReader(txtFile);
  	            createText(r,text);
  	            r.close();
  	            txtResource.text().set(text);
  	            txtResource.text().set(text);
  	            txtResource.origin().set(txtFile.getAbsolutePath());
  	            return txtResource;
  	        } catch (Exception e) {
  	            return null;
  	        }
  

• About the plain text, why do you read from the file character by character and not read the whole text and then set it to the HotText with the setString(string) method??? It will be faster and cleaner.
• This method -> userAddPlainTextFrame(); in the InsertPlainTextItem is unnecessary, it's just one line writh this line in the clicked method.
• Identation??? Ctrl+i on selected text idents it in Eclipse. Spacing, little, tine spaces between comas and expressions.
• Learn what is AutoVisualProvider, use it when you write your modules.
• You didn't test with the true runners because you didn't configure the bundles... So I told you to do it several times and you didn't do it.
• All the tabulations in the Eclipses must be 4 spaces, not 8... That is for the whole team, I think that's the reason because your identation is so strange. Fix that.
• Hmmm bad, very bad:

                  for (int i = 0; i < buffer.length(); ++i) {
                      HotUnit unit = new HotUnit(buffer.charAt(i));
                      text.replaceText(nav.getEnd(), nav.getEnd(), Arrays.asList(unit));
                  }
  

  // GOOD way:
  		text.setString(buffer.toString());
  

• The hot text itself must be refactored to be inserted through the LogicR3 events, I'm talking about the InsertTextItem. Delete the AppLogic#userInsertFrame method now when you have the provider and the LogicR3 insert.
• Don't use java assertations in tests, use assertTrue/assertFalse from the JUnit API.

1p (1h)

[meddle]

• Svn related notes:
  • You fucked up your branch... There is a sophie2-platform folder that has th emodules directory, but you have a new modules directory with your new modules in it. The new modules directory is sibling of the sophie2-platform folder in the directory tree. Something like:
    • sophie2-platform
      • modules -> old modules
    • modules -> your two new modules.
• Code related notes:
  • Again you have bad identation, I told you to set the eclipse's tabs to be 4 spaces and to use ctrl+i on a selected text to indent it. A simple solution when you are done writing a file just do ctrl+a (select everything in the file), ctrl+i (ident everything that is selected).
  • Don't comment code which is to be deleted... delete it!
  • Use the AutoVisualProvider for the InsertRtfItem.
  • And again about the @author... Write your trac identifier (diana with a small letter).
  • In the RtfLogic rename USER_IMPORT_RTF to IMPORT_RTF. It's clear that it's a user operation anyways.
  • About outer stuff there is a JavaDoc directive @see. Instead {@link javax.swing.text.StyledDocument} such things use it.
  • And now that is the main reason that I fail this implementation (see that example. I idented your code with 4 space tabs and put a new line when you do different things. Now the code is 200% more readable for other people.):
    • That thing is called swallowing exception. Never, never, never, except in special cases, very special you are allowed to catch just Exceptions for example IndexOutOfBoundsException, IOException or MyDefinedException.
    • In that case (you have other places with such code too) you must do some checks to prevent throwing exception, may be conditions using "if"s and returning special results, or if you have to deal with an Exception you catch it's real type no the arch-type Exception and throw a RuntimeException with it like a "cause" parameter.
    • I will answer you why I insist you to learn not to write such code... Milo complained a lot that in the current code there are such things, so we will have to learn not to pass them to the trunk anymore. Every developer must learn to write high quality code.

          try {
              Reader r = new FileReader(rtfFile);
              kit.read(r, doc, 0);
              r.close();
      
              JTextPane pane = new JTextPane();
              pane.setEditorKit(kit);
              pane.setDocument(doc);
      
              ApplyRtfStylesUtility.applyStyles(text, doc.getDefaultRootElement());
              rtfResource.text().set(text);
              rtfResource.origin().set(rtfFile.getAbsolutePath());
      
              return rtfResource;
          } catch (Exception e) {
              return null;
          }
      

  • If you rename or change something small, like the remaming the RtfImportUtil to RtfImportManager, you must do one of these:
    • You update the design section.
    • You add a note in the implementation section.
    • If the change is big, you must update the implementation section with a proper explanation, and not the design. The documentation of your code is important.
  • You didn't delete the RtfImportUtil... you have both RtfImportUtil and RtfImportManager...
  • Again... that code will not work with the true runners... That is not acceptable... see that check list Integration/CheckList. You must do all the stuff in it before passing a task to review if you don't want the task to fail.
  • I will show you a better way using readers for reading for example files. You can use the BufferedReader to read in buffers of chars to like char[1024]...

        BufferedReader r = 
            new BufferedReader(new FileReader(new File("/some/path")));
        r.readLine();
    

    // You can optimize your code from the PlainTextImportManager to something like this:
        public static void createText(BufferedReader r, HotText text) throws IOException {
            String current = null;
            StringBuffer str = new StringBuffer();
    
            while ((current = r.readLine()) != null) {
    	   str.append(current);
    	}
    
            text.setString(str.toString());
        }
    // It's not tested but it should work... Or something like it will work.
    

    • And you have both PlainTextImportManager and PlainTextImportUtil, and you swallow exception in the PlainTextImportManager too.
    • You asked me where is your one-row "if" without curly brackets :). I'll give you an exact answer now : in InsertTextItemLogic, lines 43-44.
    • In HotTextContentProvider:

      // Bad creation of resource ref, I told you about that, but now I'll show you with examples...
          frameContent.mainResource().set(
              ResourceRef.getReferenceByIdAndLocation(
              resource.getEntityId(), resource.getLocation())
              );
      

      // The right one:
          frameContent.mainResource().set(
              ResourceRef.getRelativeRef(
              this, parent)
              );
      

    • Again in the provider for hot text you have this assert (resource instanceof ImageResource);. And that thing works??? Did you test it, the resource must be HotTextResource, not ImageResource.
    • And you have both HtmlTextImportManager and HtmlTextImportUtil, and you swallow exception in the PlainTextImportManager too.
    • Again things like this in JUnitTests assert !(frameViews.isEmpty()); are bad... use assertFalse(frameViews.isEmpty());. That's from HtmlImportTest. Where the identation is a myth. And that is with messed up arguments : assertEquals(this.text,bf1.toString());. And the code in this test is hardly readable... :(
    • About the other two tests, you copied the logic and the same mistakes are repeated. The tests are one of the most important things, but you take them like an overhead and do them not writing carefully, copying and pasting, with one reason : the design must pass. Am I right? If you write good tests you can use them to test your use cases, they are important and helpfull.
• Please next time fix all these things I wrote... and sync with the trunk before passing for review, because the task will become too old, there is a new rule, if a task is old more than 4-5 trunk days, you must sync it before passing it to review.
  • If you have problems fixing the stuff I wrote you ask me or pap to help you.

1p (80m)

[meddle]

That time the implementation passes. I have the following notes:

• You had one Exception swallowing in the rtf logic.
• Your tests for plain and rtf import were failing, because you had started the html module instead their own. Bad -> copy-paste, I had comments about that...
• The memory problems, but they are for large files, and the text layout may be is guilty too. In the next revision may be one of the main requirements must be to fix this issue.
• Applying the styles is not working very well or at all, that is a problem it is not in the requirements for that task, so fix that in the next revision too.
• You didn't modify the main pom in the commited code, may be that thing was lost at merging, don't know...

So you updated the insert frame logic, Pap things it could be better, but he is perfectionist may be :)

3.5p (100m)

[deyan]

Batch update from file query-obsoleted.csv