[LON-CAPA-cvs] cvs: loncom /interface lonnavmaps.pm

bowersj2 lon-capa-cvs@mail.lon-capa.org
Fri, 08 Nov 2002 20:58:13 -0000


This is a MIME encoded message

--bowersj21036789093
Content-Type: text/plain

bowersj2		Fri Nov  8 15:58:13 2002 EDT

  Modified files:              
    /loncom/interface	lonnavmaps.pm 
  Log:
  Generalized cleanup and correctness check; checking for dead variables,
  dead capabilities, incorrect or misleading comments, etc.
  
  
  
--bowersj21036789093
Content-Type: text/plain
Content-Disposition: attachment; filename="bowersj2-20021108155813.txt"

Index: loncom/interface/lonnavmaps.pm
diff -u loncom/interface/lonnavmaps.pm:1.100 loncom/interface/lonnavmaps.pm:1.101
--- loncom/interface/lonnavmaps.pm:1.100	Fri Nov  8 14:10:58 2002
+++ loncom/interface/lonnavmaps.pm	Fri Nov  8 15:58:13 2002
@@ -2,7 +2,7 @@
 # The LearningOnline Network with CAPA
 # Navigate Maps Handler
 #
-# $Id: lonnavmaps.pm,v 1.100 2002/11/08 19:10:58 bowersj2 Exp $
+# $Id: lonnavmaps.pm,v 1.101 2002/11/08 20:58:13 bowersj2 Exp $
 #
 # Copyright Michigan State University Board of Trustees
 #
@@ -89,10 +89,12 @@
     $r->print(&Apache::loncommon::bodytag('Navigate Course Map','',
                                           ''));
     $r->print('<script>window.focus();</script>');
-    my $desc=$ENV{'course.'.$ENV{'request.course.id'}.'.description'};
+
     $r->print('<table border="0" cellpadding="2" cellspacing="0">');
     my $date=localtime;
     $r->print('<tr><td align="right" valign="bottom">Key:&nbsp;&nbsp;</td>');
+
+    # Print discussions and feedback header
     if ($navmap->{LAST_CHECK}) {
         $r->print('<td align="center" valign="bottom">&nbsp;&nbsp;'.
                   '<img src="/adm/lonMisc/chat.gif"> New discussion since '.
@@ -107,6 +109,7 @@
                   '</td>'); 
     }
     $r->print('</tr></table>');
+
     my $condition = 0;
     if ($ENV{'form.condition'}) {
         $condition = 1;
@@ -135,6 +138,9 @@
     # is technically not proper, but should be harmless
     my $res = $navmap->firstResource();
 
+    # These are some data tables, which make it easy to change some of
+    # of the specific visualization parameters if desired.
+
     # Defines a status->color mapping, null string means don't color
     my %colormap = 
     ( $res->NETWORK_FAILURE        => '',
@@ -192,6 +198,7 @@
     my $currenturl = $ENV{'form.postdata'};
     $currenturl=~s/^http\:\/\///;
     $currenturl=~s/^[^\/]+//;
+
     # alreadyHere allows us to only open the maps necessary to view
     # the current location once, while at the same time remembering
     # the current location. Without that check, the user would never
@@ -210,16 +217,14 @@
     # Here's a simple example of the iterator.
     # Preprocess the map: Look for current URL, force inlined maps to display
 
-    # This currently does very little...
-    #my $mapEventualIterator = Apache::lonnavmaps::iterator->new($navmap, undef, undef, {},
-    #undef, $condition);
-
-    my $mapIterator = $navmap->getIterator(undef, undef, {}, 1);
+    my $mapIterator = $navmap->getIterator(undef, undef, \%filterHash, 1);
     my $found = 0;
     my $depth = 1;
     my $currentUrlIndex = 0; # keeps track of when the current resource is found,
                              # so we can back up a few and put the anchor above the
                              # current resource
+    my $currentUrlDelta = 5; # change this to change how many resources are displayed
+                             # before the current resource when using #current
     $mapIterator->next(); # discard the first BEGIN_MAP
     my $curRes = $mapIterator->next();
     my $counter = 0;
@@ -237,8 +242,12 @@
             # show it by making sure the containing maps
             # are open.
 
+            # This is why we have to use the main iterator instead of the
+            # potentially faster DFS: The count has to be the same, so
+            # the order has to be the same, which DFS won't give us.
             $currentUrlIndex = $counter;
             
+            # Ensure the parent maps are open
             for my $map (@{$mapStack}) {
                 if ($condition) {
                     undef $filterHash{$map->map_pc()};
@@ -286,15 +295,16 @@
 
     $counter = 0;
 
+    # Print the 'current' anchor here if it would fall off the top
+    if ($currentUrlIndex - $currentUrlDelta < 0) {
+        $r->print('<a name="current" />');
+    }
+
     while ($depth > 0) {
-        # If we're in a new style course, and this is a BEGIN_MAP, END_MAP, or
-        # map resource and the stack depth is only one, just plain ignore this resource
-        # entirely. (This has the effect of inlining the resources in that map
-        # in the nav map.)
+        # If this is an inlined map, cancel the shift to the right,
+        # which has the effect of making the map look inlined
         if ($inlineTopLevelMaps && scalar(@{$mapIterator->getStack()}) == 1 &&
             ref($curRes) && $curRes->is_map()) {
-            # We let the normal depth stuff occur, but we need to shift everything
-            # over by one to the left to make it look right.
             $deltadepth = -1;
             $curRes = $mapIterator->next();
             next;
@@ -319,10 +329,11 @@
         if ($depth == 1) { $deltadepth = 0; } # we're done shifting, because we're
                                               # out of the inlined map
 
-        # Is this resource being blotted out?
+        # Is this resource being ignored because it is in a random-out
+        # map and it was not selected?
         if (ref($curRes) && !advancedUser() && $curRes->randomout()) {
             $curRes = $mapIterator->next();
-            next; # and totally ignore this resource
+            next; # if yes, then just ignore this resource
         }
 
         if (ref($curRes) && $curRes->src()) {
@@ -333,14 +344,15 @@
             my $condensed = 0;
                 
             if ($curRes->is_problem()) {
-                # Is it multipart?
 
+                # Is it multipart?
                 if ($multipart) {
                     # If it's multipart, see if part 0 is "open"
                     # if it is, display all parts, if it isn't,
                     # just display first
                     if (!$curRes->opendate("0")) {
-                        @parts = ("0"); # just display the zero-th part
+                        # no parts are open, display as one part
+                        @parts = ("0");
                         $condensed = 1;
                     } else {
                         # Otherwise, only display part 0 if we want to 
@@ -348,9 +360,6 @@
                         if ($curRes->hasDiscussion() || $curRes->getFeedback()) {
                             shift @parts;
                         } else {
-                            # If there's discussion or feedback, that counts
-                            # as a difference, so display the parts.
-                            
                             # Now, we decide whether to condense the
                             # parts due to similarity
                             my $status = $curRes->status($parts[1]);
@@ -391,11 +400,11 @@
             } else {
                 $parts[0] = "0"; # this is to get past foreach loop below
                  # you can consider a non-problem resource as a resource
-                  # with only one part without loss
+                  # with only one part without loss, and it simplifies the looping
             }
 
             # Is it a multipart problem with a single part, now in 
-            # @parts with "0" filtered out? If so, forget it's a multi-part
+            # @parts with "0" filtered out? If so, 'forget' it's a multi-part
             # problem and treat it like a single-part problem.
             if ( scalar(@parts) == 1 ) {
                 $multipart = 0;
@@ -403,27 +412,31 @@
 
             # Display one part, in event of network error.
             # If this is a single part, we can at least show the correct
-            # status, but if it's multipart, we're lost.
+            # status, but if it's multipart, we're lost, since we can't
+            # retreive the metadata to count the parts
             if ($curRes->{RESOURCE_ERROR}) {
                 @parts = ("0");
             }
 
-            # Step two: Show the parts
+            # Step Two: Print the actual data.
+
+            # For each part we intend to display...
             foreach my $part (@parts) {
 
                 my $deltalevel = 0; # for inserting the branch icon
                 my $nonLinkedText = ""; # unlinked stuff after title
                 
-                # For each thing we're displaying...
-
                 my $stack = $mapIterator->getStack();
                 my $src = getLinkForResource($stack);
 
+                # Pass the correct symb on the querystring, so the
+                # remote will figure out where we are if we click a link
                 my $srcHasQuestion = $src =~ /\?/;
                 my $link = $src.
                     ($srcHasQuestion?'&':'?') .
                     'symb='.&Apache::lonnet::escape($curRes->symb()).
                     '"';
+
                 my $title = $curRes->title();
                 if (!$title) {
                     $title = $curRes->src();
@@ -433,11 +446,10 @@
                 my $newBranchText = "";
 
                 # If this is a new branch, label it so
-                # (temporary, this should be an icon w/ alt text)
                 if ($isNewBranch) {
                     $newBranchText = "<img src=\"/adm/lonIcons/branch.gif\" border=\"0\">";
                     $isNewBranch = 0;
-                    $deltalevel = 1;
+                    $deltalevel = 1; # reserves space for the branch icon
                 }
 
                 # links to open and close the folders
@@ -509,11 +521,8 @@
                 # FIRST COL: The resource indentation, branch icon, name, and anchor
                 $r->print("  <tr bgcolor=\"$backgroundColor\"><td align=\"left\" valign=\"center\" width=\"60%\">\n");
 
-                # anchor for current resource... - 5 is deliberate: If it's that
-                # high on the screen, don't bother focusing on it. Also this will
-                # print multiple anchors if this is an expanded multi-part problem...
-                # who cares?
-                if ($counter == $currentUrlIndex - 5) {
+                # Print the anchor if necessary
+                if ($counter == $currentUrlIndex - $currentUrlDelta) {
                     $r->print('<a name="current" />');
                 }
 
@@ -543,18 +552,15 @@
                 }
 
                 $r->print("  $curMarkerBegin<a href=\"$link\">$title$partLabel</a> $curMarkerEnd $nonLinkedText");
-                #$r->print(" TDV:" . $curRes->{DATA}->{TOP_DOWN_VAL}); # temp
-                #$r->print(" BUV:" . $curRes->{DATA}->{BOT_UP_VAL}); # temp
-                #$r->print(" DD:" . $curRes->{DATA}->{DISPLAY_DEPTH}); # temp
 
                 if ($curRes->{RESOURCE_ERROR}) {
                     $r->print(&Apache::loncommon::help_open_topic ("Navmap_Host_Down",
                                               '<font size="-1">Host down</font>'));
                     }
 
+                # SECOND COL: Is there text, feedback, errors??
                 my $discussionHTML = ""; my $feedbackHTML = "";
 
-                # SECOND COL: Is there text, feedback, errors??
                 if ($curRes->hasDiscussion()) {
                     $discussionHTML = $linkopen .
                         '<img border="0" src="/adm/lonMisc/chat.gif" />' .
@@ -577,7 +583,8 @@
 
                 # Is this the first displayed part of a multi-part problem
                 # that has not been condensed, so we should suppress these two
-                # columns?
+                # columns so we don't display useless status info about part
+                # "0"?
                 my $firstDisplayed = !$condensed && $multipart && $part eq "0";
 
                 # THIRD COL: Problem status icon
@@ -595,7 +602,6 @@
                 }
 
                 # FOURTH COL: Text description
-                #$r->print("<td $colorizer align=\"right\" valign=\"center\">\n");
                 $r->print("<td align=\"right\" valign=\"center\">\n");
                 
                 if ($curRes->kind() eq "res" &&
@@ -727,6 +733,7 @@
     }
 }
 
+# This puts a human-readable name on the ENV variable.
 sub advancedUser {
     return $ENV{'user.adv'};
 }
@@ -772,17 +779,18 @@
         $delta = -$delta;
     }
 
-    # Is it in the future?
     if ( $delta > 0 ) {
-        # Is it less then a minute away?
+
         my $tense = $inPast ? " ago" : "";
         my $prefix = $inPast ? "" : "in ";
+        
+        # Less then a minute
         if ( $delta < $minute ) {
             if ($delta == 1) { return "${prefix}1 second$tense"; }
             return "$prefix$delta seconds$tense";
         }
 
-        # Is it less then an hour away?
+        # Less then an hour
         if ( $delta < $hour ) {
             # If so, use minutes
             my $minutes = floor($delta / 60);
@@ -876,7 +884,7 @@
     $self->{GENERATE_COURSE_USER_OPT} = shift;
     $self->{GENERATE_EMAIL_DISCUSS_STATUS} = shift;
 
-    # Resource cache stores navmapresource's as we reference them. We generate
+    # Resource cache stores navmap resources as we reference them. We generate
     # them on-demand so we don't pay for creating resources unless we use them.
     $self->{RESOURCE_CACHE} = {};
 
@@ -1029,8 +1037,7 @@
 sub getIterator {
     my $self = shift;
     my $iterator = Apache::lonnavmaps::iterator->new($self, shift, shift,
-                                                     shift, undef, shift, 
-                                                     $ENV{'form.direction'});
+                                                     shift, undef, shift);
     return $iterator;
 }
 
@@ -1121,14 +1128,8 @@
     return $self->getById($firstResource);
 }
 
-# -------------------------------------------- Figure out a cascading parameter
-#
-# For this function to work
-#
-# * parmhash needs to be tied
-# * courseopt and useropt need to be initialized for this user and course
-#
-
+# Parmval reads the parm hash and cascades the lookups. parmval_real does
+# the actual lookup; parmval caches the results.
 sub parmval {
     my $self = shift;
     my ($what,$symb)=@_;
@@ -1230,10 +1231,6 @@
 
 package Apache::lonnavmaps::iterator;
 
-# This package must precede "navmap" because "navmap" uses it. In
-# order to keep the documentation order straight, the iterator is documented
-# after the navmap object.
-
 =pod
 
 =back
@@ -1254,9 +1251,9 @@
 
 =over 4
 
-=item B<getIterator>(firstResource, finishResource, filterHash, condition, direction): All parameters are optional. firstResource is a resource reference corresponding to where the iterator should start. It defaults to navmap->firstResource() for the corresponding nav map. finishResource corresponds to where you want the iterator to end, defaulting to navmap->finishResource(). filterHash is a hash used as a set containing strings representing the resource IDs, defaulting to empty. Condition is a 1 or 0 that sets what to do with the filter hash: If a 0, then only resource that exist IN the filterHash will be recursed on. If it is a 1, only resources NOT in the filterHash will be recursed on. Defaults to 0, which is to say, do not recurse unless explicitly asked to. Direction specifies which direction to recurse, either FORWARD or BACKWARD, with FORWARD being default.
+=item * B<getIterator>(firstResource, finishResource, filterHash, condition): All parameters are optional. firstResource is a resource reference corresponding to where the iterator should start. It defaults to navmap->firstResource() for the corresponding nav map. finishResource corresponds to where you want the iterator to end, defaulting to navmap->finishResource(). filterHash is a hash used as a set containing strings representing the resource IDs, defaulting to empty. Condition is a 1 or 0 that sets what to do with the filter hash: If a 0, then only resource that exist IN the filterHash will be recursed on. If it is a 1, only resources NOT in the filterHash will be recursed on. Defaults to 0.
 
-Thus, by default, all resources will be shown. Change the condition to a 1 without changing the hash, and only the top level of the map will be shown. Changing the condition to 1 and including some values in the hash will allow you to selectively examine parts of the navmap, while leaving it on 0 and adding things to the hash will allow you to selectively ignore parts of the nav map. See the handler code for examples: By default, the condition is 0 and all folders are closed unless explicitly opened. Clicking "Show All Resources" will use a condition of 1 and an empty filterHash, resulting in all resources being shown.
+Thus, by default, only top-level resources will be shown. Change the condition to a 1 without changing the hash, and all resources will be shown. Changing the condition to 1 and including some values in the hash will allow you to selectively suppress parts of the navmap, while leaving it on 0 and adding things to the hash will allow you to selectively add parts of the nav map. See the handler code for examples.
 
 The iterator will return either a reference to a resource object, or a token representing something in the map, such as the beginning of a new branch. The possible tokens are:
 
@@ -1756,7 +1753,7 @@
 
 B<Metadata Retreival>
 
-These are methods that help you retrieve metadata about the resource:
+These are methods that help you retrieve metadata about the resource: Method names are based on the fields in the compiled course representation.
 
 =over 4
 
@@ -1766,7 +1763,7 @@
 
 =item * B<kind>: Returns the kind of the resource from the compiled nav map.
 
-=item * B<randomout>: Returns true if this resource was chosen to NOT be shown to the user by the random map selection feature.
+=item * B<randomout>: Returns true if this resource was chosen to NOT be shown to the user by the random map selection feature. In other words, this is usually false.
 
 =item * B<randompick>: Returns true for a map if the randompick feature is being used on the map. (?)
 
@@ -1826,7 +1823,7 @@
 
 =item * B<is_problem>: Returns true if the resource is a problem.
 
-=item * B<is_sequence>: Returns true if the resource sequence.
+=item * B<is_sequence>: Returns true if the resource is a sequence.
 
 =back
 
@@ -1855,12 +1852,7 @@
     return ($src =~ /sequence$/);
 }
 
-
-
-
-
-# Move this into POD: In order to use these correctly, courseopt
-# and useropt need to be generated
+# Private method: Shells out to the parmval in the nav map, handler parts.
 sub parmval {
     my $self = shift;
     my $what = shift;
@@ -2143,7 +2135,7 @@
 
 =head2 Resource Status
 
-Problem resources have status information, reflecting their various dates and completion statuses. You can obtain this information and import symbolic constants to represent the status.
+Problem resources have status information, reflecting their various dates and completion statuses. 
 
 There are two aspects to the status: the date-related information and the completion information.
 
@@ -2160,6 +2152,10 @@
     ... use it here ...
  }
 
+Which you use depends on exactly what you are looking for. The status() function has been optimized for the nav maps display and may not precisely match what you need elsewhere.
+
+The symbolic constants shown below can be accessed through the resource object: $res->OPEN.
+
 =over 4
 
 =item * B<getDateStatus>($part): ($part defaults to 0). A convenience function that returns a symbolic constant telling you about the date status of the part. The possible return values are:
@@ -2296,7 +2292,7 @@
 
 B<Composite Status>
 
-Along with directly returning the date or completion status, the resource object includes a convenience function B<status>() that will combine the two status tidbits into one composite status that can represent the status of the resource as a whole. The precise logic is documented in the comments of the status method. The following results may be returned, all available as methods on the resource object ($res->NETWORK_FAILURE()):
+Along with directly returning the date or completion status, the resource object includes a convenience function B<status>() that will combine the two status tidbits into one composite status that can represent the status of the resource as a whole. The precise logic is documented in the comments of the status method. The following results may be returned, all available as methods on the resource object ($res->NETWORK_FAILURE):
 
 =over 4
 
@@ -2400,15 +2396,14 @@
 
 =over 4
 
-=item * B<getNext>($alreadySeenHashRef): Retreive an array of the possible next resources after this one. Always returns an array, even in the one- or zero-element case. The "alreadySeenHashRef" is an optional parameter that can be passed in to the method. If $$alreadySeenHashRef{$res->id()} is true in that hash, getNext will not return it in the list. In other words, you can use it to suppress resources you've already seen in the getNext method directly.
+=item * B<getNext>(): Retreive an array of the possible next resources after this one. Always returns an array, even in the one- or zero-element case. 
 
-=item * B<getPrevious>($alreadySeenHashRef): Retreive an array of the possible previous resources from this one. Always returns an array, even in the one- or zero-element case. $alreadySeenHashRef is the same as in getNext.
+=item * B<getPrevious>(): Retreive an array of the possible previous resources from this one. Always returns an array, even in the one- or zero-element case. 
 
 =cut
 
 sub getNext {
     my $self = shift;
-    my $alreadySeenHash = shift;
     my @branches;
     my $to = $self->to();
     foreach my $branch ( split(/,/, $to) ) {
@@ -2416,21 +2411,18 @@
         my $next = $choice->goesto();
         $next = $self->{NAV_MAP}->getById($next);
 
-        # Don't remember it if we've already seen it or if
-        # the student doesn't have browse priviledges
+        # Don't remember it if the student doesn't have browse priviledges
+        # future note: this may properly belong in the client of the resource
         my $browsePriv = &Apache::lonnet::allowed('bre', $self->src);
-        if (!defined($alreadySeenHash) ||
-            !defined($alreadySeenHash->{$next->{ID}}) ||
-            ($browsePriv ne '2' && $browsePriv ne 'F')) {
-                push @branches, $next;
-            }
+        if ($browsePriv ne '2' && $browsePriv ne 'F') {
+            push @branches, $next;
+        }
     }
     return \@branches;
 }
 
 sub getPrevious {
     my $self = shift;
-    my $alreadySeenHash = shift;
     my @branches;
     my $from = $self->from();
     foreach my $branch ( split /,/, $from) {
@@ -2438,12 +2430,10 @@
         my $prev = $choice->comesfrom();
         $prev = $self->{NAV_MAP}->getById($prev);
 
-        # Skip it if we've already seen it or the user doesn't have
-        # browse privs
+        # Don't remember it if the student doesn't have browse priviledges
+        # future note: this may properly belong in the client of the resource
         my $browsePriv = &Apache::lonnet::allowed('bre', $self->src);
-        if (!defined($alreadySeenHash) ||
-            !defined($alreadySeenHash->{$prev->{ID}}) ||
-            ($browsePriv ne '2' && $browsePriv ne 'F')) {
+        if ($browsePriv ne '2' && $browsePriv ne 'F') {
             push @branches, $prev;
         }
     }

--bowersj21036789093--