Ticket #275 (closed task: fixed)

Opened 5 years ago

Last modified 5 years ago

Code Review

Reported by: Joel Owned by: Joel
Priority: major Milestone: Eraser 6.1/6.2
Component: Core Version:
Keywords: Cc:
Processor Architecture: Blocked By: #276, #284, #307, #309, #320
Blocking: #262 Operating System:

Description (last modified by Joel) (diff)

  • Find a way to return the status of the erase from the DirectExecutor? (like calling Invoke)
  • Throw exceptions only when really necessary (such as in the Unused Space erasure code, maybe?)
  • IDisposable.Dispose is supposed to be reentrant
  • Fix all MessageBox.Show calls to include RightAlign and the RtlReading members of the MessageBoxOptions enumeration. http://msdn.microsoft.com/en-us/library/ms182191.aspx

From #284:

Change History

comment:1 Changed 5 years ago by Joel

  • Define the ERROR_* constants from Win32 instead of using hardcoded numbers for readability
    • Perhaps write a function to replace the ExceptionFromHRESULT marshal function - the Util assembly usually throws strange exceptions
  • Better Opportunist Command Line program behaviour
  • Rewrite the Log viewer form!
  • Rewrite the Updater/BlackBox? uploader -- unify them (Factor out the MultipartFormDataBuilder? class)
  • Erasure methods should be based on an interface: IErasureMethod, IChainedErasureMethod
    • Guid property of all Erasure methods should be static; implement the Guid property as an interface member instead
  • Find a way to return the status of the erase from the DirectExecutor? (like calling Invoke)
  • Replace all the new EventHandler?<X> calls - no need for that
  • IList<T> vs. ICollection<T>?
  • Combine FileSystem?.DeleteFile? and FileSystem?.DeleteFolder?
  • Use System.Func generics instead of out own types of delegates

comment:2 Changed 5 years ago by Joel

  • Type changed from defect to task

comment:3 Changed 5 years ago by Joel

  • Owner set to Joel
  • Status changed from new to accepted

comment:4 Changed 5 years ago by Joel

  • Blocked By 272, 276, 284 added

comment:5 Changed 5 years ago by Joel

  • Blocked By 90 added

comment:6 Changed 5 years ago by Joel

  • Blocked By 272 removed

comment:7 Changed 5 years ago by Joel

Move helper classes like the ProgressManager? classes in #90 to the Util library.

comment:8 Changed 5 years ago by Joel

The updater classes are really messy! I'll need to restructure the code somehow.

comment:9 Changed 5 years ago by Joel

  • Blocked By 307 added

comment:9 Changed 5 years ago by Joel

  • Description modified (diff)
  • Blocked By 307 removed

comment:10 Changed 5 years ago by Joel

  • Blocked By 307 added

comment:11 Changed 5 years ago by Joel

  • Blocked By 90 removed

comment:12 Changed 5 years ago by Joel

  • Blocked By 90 added

(In #90) There's a standard method of broadcasting the events to interested listeners since r1512.

comment:14 Changed 5 years ago by Joel

  • Description modified (diff)

These points belong to #284

comment:15 Changed 5 years ago by Joel

  • Description modified (diff)

The System.Func generics don't really help -- especially since we are defining delegates of a particular nature (usually event handling)

comment:16 Changed 5 years ago by Joel

  • Description modified (diff)

Command line/GUI program code rewrite begins with r1521 and completed with r1524

comment:17 Changed 5 years ago by Joel

  • Description modified (diff)

Log viewer form reviewed in r1526

comment:18 Changed 5 years ago by Joel

  • Description modified (diff)

"new EventHandler?" was removed from Eraser in r1527

comment:19 Changed 5 years ago by Joel

  • Description modified (diff)

ICollections replaced with ILists, where applicable in r1528

comment:20 Changed 5 years ago by Joel

  • Description modified (diff)

FileSystem.DeleteFile and FileSystem.DeleteFolder seems to be able to be combined only in our case in WindowsFileSystem, so combine it there first. Fixed in r1529

comment:21 Changed 5 years ago by Joel

  • Blocked By 90 removed

comment:22 Changed 5 years ago by Joel

  • Blocked By 272 removed

Belongs to #284

comment:23 Changed 5 years ago by Joel

  • Blocked By 317 added

comment:24 Changed 5 years ago by Joel

  • Description modified (diff)
  • Blocked By 317 removed

These belong in #307

comment:25 Changed 5 years ago by Joel

  • Blocked By 320 added

comment:26 Changed 5 years ago by Joel

  • Description modified (diff)

comment:27 Changed 5 years ago by Joel

  • Description modified (diff)

comment:28 Changed 5 years ago by Joel

  • Description modified (diff)

comment:29 Changed 5 years ago by Joel

  • Description modified (diff)

comment:30 Changed 5 years ago by Joel

  • Description modified (diff)

comment:31 Changed 5 years ago by Joel

  • Description modified (diff)

comment:32 Changed 5 years ago by Joel

  • Description modified (diff)

comment:33 Changed 5 years ago by Joel

  • Description modified (diff)

Fixed in r1766

comment:34 Changed 5 years ago by Joel

  • Description modified (diff)

Exceptions generally reviewed in r1767; exceptions are thrown only when we can't do anything about the error. Also, developer-visible exceptions are now non-localisable strings; user-visible exceptions should be changed to log messages.

comment:35 Changed 5 years ago by Joel

  • Description modified (diff)

Fixed in r1768

comment:36 Changed 5 years ago by Joel

  • Description modified (diff)

Moved to #323

comment:37 Changed 5 years ago by Joel

  • Description modified (diff)

Implemented in r1770 and r1772.

comment:38 Changed 5 years ago by Joel

  • Blocked By 309 added

comment:39 Changed 5 years ago by Joel

  • Blocking 262 added

comment:40 Changed 5 years ago by Joel

  • Description modified (diff)

Already removed.

comment:41 Changed 5 years ago by Joel

  • Description modified (diff)

The design doesn't seem to work too well since we have the notion of queued tasks.

comment:42 Changed 5 years ago by Joel

  • Description modified (diff)

These belong to #277

comment:43 Changed 5 years ago by Joel

  • Description modified (diff)

The rest aren't important now, I'll prepare to close this ticket.

comment:44 Changed 5 years ago by Joel

  • Status changed from accepted to closed
  • Resolution set to fixed

Completed in r1803

Note: See TracTickets for help on using tickets.